Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

EncodingAttr to track type of source op #17756

Merged
merged 1 commit into from
Jul 9, 2024
Merged

EncodingAttr to track type of source op #17756

merged 1 commit into from
Jul 9, 2024

Conversation

lialan
Copy link
Contributor

@lialan lialan commented Jun 27, 2024

This is to get encoding ready for ops other than MATMUL.

Resolves #17604

@lialan lialan force-pushed the lialan/op_type branch 2 times, most recently from 7446669 to 2ae291c Compare June 27, 2024 16:38
@lialan
Adding new optype
695182c 
Signed-off-by: Alan Li <[email protected]>
@lialan lialan requested a review from bjacob June 27, 2024 22:04
@lialan lialan marked this pull request as ready for review June 27, 2024 22:04
MaheshRavishankar
MaheshRavishankar reviewed Jun 27, 2024
View reviewed changes
Copy link
Contributor

@MaheshRavishankar MaheshRavishankar left a comment

Choose a reason for hiding this comment

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

Cant we use the indexing maps to deduce that this is a matmul?

@hanhanW
Copy link
Contributor

hanhanW commented Jun 27, 2024

Cant we use the indexing maps to deduce that this is a matmul?

I think it is possible. We can use linalg::inferContractionDims(maps) to see if it is a contraction op.

@lialan
Copy link
Contributor Author

lialan commented Jul 6, 2024

@hanhanW @MaheshRavishankar if we can figure out by simply looking at the source op type, then we probably don't need this change?

@MaheshRavishankar
Copy link
Contributor

MaheshRavishankar commented Jul 9, 2024 •
edited
Loading

@hanhanW @MaheshRavishankar if we can figure out by simply looking at the source op type, then we probably don't need this change?

I was talking to Benoit about this. I think he clarified something. A linalg op is characterized with three things, iterator types, indexing maps and region. We dont care about iterator types here for now in the encoding, and we are considering how to represent the indexing maps. We might need to represent the region in the attribute though. So saying MATMUL is OK I think. This plus "element types" gives us description of the body. of the op. So this change might be OK

MaheshRavishankar
MaheshRavishankar approved these changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@MaheshRavishankar MaheshRavishankar left a comment

Choose a reason for hiding this comment

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

Ok, this change itself is OK for me.

compiler/src/iree/compiler/Codegen/VMVX/test/select_lowering_strategy.mlir
@@ -168,7 +168,7 @@ func.func @elem_pack_ukernels() attributes {hal.executable.target = #executable_
%cst = arith.constant 0.000000e+00 : f32
%c0 = arith.constant 0 : index
%0 = hal.interface.binding.subspan set(0) binding(0) type(storage_buffer) alignment(64) offset(%c0) flags(ReadOnly) : !flow.dispatch.tensor<readonly:tensor<1024x2048xf32>>
%1:2 = iree_codegen.query_tile_sizes tensor<1024x2048xf32, #iree_encoding.encoding<operand_index = 0, element_types = [f32, f32, f32], original_type = tensor<1024x2048xf32>>> -> index, index
%1:2 = iree_codegen.query_tile_sizes tensor<1024x2048xf32, #iree_encoding.encoding<operand_index = 0, op_type = matmul, element_types = [f32, f32, f32], original_type = tensor<1024x2048xf32>>> -> index, index
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated, but I dont know what original_type is for. We should really drop it I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's needed by current path because we could generate query_tile_sizes + pad ops. IMO, we will deprecate it in the new data-tiling flow that we're building now.

@lialan lialan merged commit 3f6bf8c into iree-org:main Jul 9, 2024
50 of 51 checks passed
@lialan lialan deleted the lialan/op_type branch July 9, 2024 01:35
LLITCHEV pushed a commit to LLITCHEV/iree that referenced this pull request Jul 30, 2024
@lialan @LLITCHEV
EncodingAttr to track type of source op (iree-org#17756)
3bbcb93 
This is to get encoding ready for ops other than MATMUL.

Resolves iree-org#17604

Signed-off-by: Alan Li <[email protected]>
Signed-off-by: Lubo Litchev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaheshRavishankar MaheshRavishankar MaheshRavishankar approved these changes

@bjacob bjacob Awaiting requested review from bjacob bjacob is a code owner

@hanhanW hanhanW Awaiting requested review from hanhanW hanhanW is a code owner

@benvanik benvanik Awaiting requested review from benvanik benvanik is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Let EncodingAttr track the op type
3 participants
@lialan @hanhanW @MaheshRavishankar