Skip to content

Conversation

@hanhanW
Copy link
Contributor

@hanhanW hanhanW commented Oct 29, 2025

The revision implements LayoutMaterializerAttr::convertType for GPUPaddingResolverAttr; updates the MaterializeInterfaceBindingEncoding pattern to use interface methods to materialize the bindings.

It also fixes a bug in @dynamic_set_zero_pad_encoding_and_store test that misses dynamic dimensions in binding ops.

It is a step towards #20160

@hanhanW hanhanW requested review from jtuyls and kuhar October 29, 2025 20:41
@hanhanW hanhanW requested a review from Max191 as a code owner October 29, 2025 20:41
@hanhanW
Copy link
Contributor Author

hanhanW commented Oct 29, 2025

I'll delete MaterializeEncodingIntoPaddingPass and update the lit tests (that should get the resolver from executable config) in a follow-up.

@hanhanW hanhanW requested a review from Copilot October 29, 2025 20:42
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the encoding materialization infrastructure to consolidate type conversion logic from MaterializeEncodingIntoPadding.cpp into the external models in GPUEncodingExternalModels.cpp. The main goal is to implement the convertType interface method directly in the GPU padding resolver attribute, eliminating the need for a separate custom type converter and pattern for padding-specific encoding.

  • Moves getPadLayout and getPaddedType helper functions from MaterializeEncodingIntoPadding.cpp to GPUEncodingExternalModels.cpp
  • Implements convertType method in GPUPadEncodingLayoutMaterializerAttr to handle type conversion for padding encodings
  • Simplifies MaterializeEncodingTypeConverter by removing padding-specific special cases
  • Updates MaterializeInterfaceBindingEncoding pattern to use the unified type conversion approach

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
GPUEncodingExternalModels.cpp Adds getPadLayout and getPaddedType helper functions and implements convertType method for GPUPadEncodingLayoutMaterializerAttr
materialize_encoding_into_padding.mlir Updates test expectations to include dynamic dimensions in dispatch tensor types
MaterializeEncodingPatterns.cpp Refactors MaterializeInterfaceBindingEncoding to use generic type converter interface and compute dynamic dimensions via getOffsetsSizesStrides
MaterializeEncodingIntoPadding.cpp Removes custom type converter and pattern implementations that are now handled by the external model
EncodingUtils.cpp Removes padding-specific special cases from type converter, delegating all type conversion to the layout attribute's convertType method

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: hanhanW <[email protected]>
@hanhanW hanhanW requested a review from kuhar October 30, 2025 05:05
@hanhanW
Copy link
Contributor Author

hanhanW commented Oct 30, 2025

Thanks @kuhar for the review; I'm fixing related code in #22471

Copy link
Contributor

@jtuyls jtuyls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return IREE::TensorExt::DispatchTensorType::get(
dispatchTensorType.getAccess(), type);
})
.Default([&](Type type) { return type; });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.Default([&](Type type) { return type; });
.Default([](Type type) { return type; });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh thanks, somehow I missed it.

Type convertType(Attribute attr, Type type) const {
auto layoutAttr = cast<IREE::Encoding::LayoutResolverAttr>(attr);
return TypeSwitch<Type, Type>(type)
.Case([&](RankedTensorType type) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.Case([&](RankedTensorType type) {
.Case([](RankedTensorType type) {

@hanhanW hanhanW enabled auto-merge (squash) October 30, 2025 16:41
@hanhanW hanhanW merged commit 166cda3 into iree-org:main Oct 30, 2025
35 of 45 checks passed
@hanhanW hanhanW deleted the pad-resolver-convert-type branch October 30, 2025 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants