Change LinalgExt::EncodingAttr from enum to structured. #14336
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bjacob
requested review from
hanhanW,
MaheshRavishankar,
dcaballe and
benvanik
as code owners
bjacob
force-pushed
the
bjacob-encoding-structured
branch
from
1d2d79a to
23bd44c
Compare
|
We had an issue with CI triggering. Can you sync this past 68350cb to run presubmits? |
bjacob
force-pushed
the
bjacob-encoding-structured
branch
3 times, most recently
from
b822caa to
ba1ec4a
Compare
benvanik
approved these changes
Jul 10, 2023
|
@MaheshRavishankar @hanhanW do you want to take a look? |
hanhanW
approved these changes
Jul 10, 2023
compiler/src/iree/compiler/Codegen/LLVMCPU/LLVMCPULowerToUKernels.cpp
Outdated
Show resolved
Hide resolved
compiler/src/iree/compiler/Codegen/LLVMCPU/LLVMCPUMaterializeEncodingPass.cpp
Outdated
Show resolved
Hide resolved
compiler/src/iree/compiler/Codegen/VMVX/VMVXMaterializeEncodingPass.cpp
Outdated
Show resolved
Hide resolved
bjacob
force-pushed
the
bjacob-encoding-structured
branch
from
ba1ec4a to
5da9f45
Compare
Ill take a look. Could you wait till EoD for me to review. This is on top of my stack. |
@MaheshRavishankar - sure, no problem. |
bjacob
force-pushed
the
bjacob-encoding-structured
branch
from
5da9f45 to
034c1c9
Compare
MaheshRavishankar
requested changes
Jul 10, 2023
...-external-projects/iree-dialects/include/iree-dialects/Dialect/LinalgExt/IR/LinalgExtBase.td
Outdated
Show resolved
Hide resolved
| def MATMUL_BF16BF16F32 : I32EnumAttrCase<"MATMUL_BF16BF16F32", 4>; | ||
| def MATMUL_BF16BF16BF16 : I32EnumAttrCase<"MATMUL_BF16BF16BF16", 5>; | ||
|
|
||
| def EncodingOpKind |
There was a problem hiding this comment.
Would it better to say something like EncodingUserKind? or just UserInfo cause it is encoding information about the user of the tensor (in this case the linalg.matmul)
There was a problem hiding this comment.
I have updated this PR with a tentative renaming along these lines, as a separate commit c658894 so it's easy to change into anything else, particularly as it was generated by a command line:
git grep --name-only -e '\(\bop_kind\b\|[oO]pKind\b\)' | grep -v dispatch_profiler | xargs sed -i 's/\bop_kind\b/user/g;s/opKind/user/g;s/OpKind/User/g'
So this is doing User not UserKind but we can do that as well. See my comment at https://discord.com/channels/689900678990135345/690274711523164166/1128034873718878290
There was a problem hiding this comment.
Note: avoiding the Info terminology because that is already how we refer to the result of MaterializeEncoding (MaterializeEncodingInfo is a struct holding materialized tile sizes).
bjacob
force-pushed
the
bjacob-encoding-structured
branch
from
034c1c9 to
c658894
Compare
|
Cool! Thanks Benoit for doing this! |
MaheshRavishankar
approved these changes
Jul 10, 2023
nhasabni
pushed a commit
to plaidml/iree
that referenced
this pull request
Aug 24, 2023
This has long been discussed, as the enum was just an initial design shortcut, but the number of enum cases was already getting uncomfortable due to the multiple dimensions there. This is a step in a chain towards fixing iree-org#11632. The reason is that in order to properly specialize for narrow matmul cases in `MaterializeEncoding`, selecting adequately narrow matmul kernels that avoid widening the entire matmul problem at hand, we will need to know there the original (pre-padding) matrix shape. Since `MaterializeEncoding` is a type-conversion, not just an ordinary rewrite pattern, this information will need to be encoded in types --- we won't just be able to walk from a value to its defining op to find the pre-padding value, there just aren't values there. So I will want to add the pre-padding shape (or type) to EncodingAttr. This is a step towards that: by making EncodingAttr a data structure, it's easy then to add another field. By contrast, if it's still an enum, the combinatorics get out of hand.
bjacob
added a commit
that referenced
this pull request
Oct 16, 2023
This has been discussed for a while: ever since #14336 made encodings a data structure, it was an odd remnant that we were still encoding the element types tuple in the user enum. This was cumbersome, and resurfaced in every design discussion as it looked like something that wasn't scaling with new data types. Concretely, this is good to fix ahead of adding `i16xi16` and `i16xi4` data-tiling support (#15158).
bjacob
added a commit
to bjacob/iree
that referenced
this pull request
Oct 16, 2023
…-org#15182) This has been discussed for a while: ever since iree-org#14336 made encodings a data structure, it was an odd remnant that we were still encoding the element types tuple in the user enum. This was cumbersome, and resurfaced in every design discussion as it looked like something that wasn't scaling with new data types. Concretely, this is good to fix ahead of adding `i16xi16` and `i16xi4` data-tiling support (iree-org#15158).
ramiro050
pushed a commit
to ramiro050/iree
that referenced
this pull request
Dec 19, 2023
…-org#15182) This has been discussed for a while: ever since iree-org#14336 made encodings a data structure, it was an odd remnant that we were still encoding the element types tuple in the user enum. This was cumbersome, and resurfaced in every design discussion as it looked like something that wasn't scaling with new data types. Concretely, this is good to fix ahead of adding `i16xi16` and `i16xi4` data-tiling support (iree-org#15158).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This has long been discussed, as the enum was just an initial design shortcut, but the number of enum cases was already getting uncomfortable due to the multiple dimensions there.
This is a step in a chain towards fixing #11632. The reason is that in order to properly specialize for narrow matmul cases in
MaterializeEncoding, selecting adequately narrow matmul kernels that avoid widening the entire matmul problem at hand, we will need to know there the original (pre-padding) matrix shape. SinceMaterializeEncodingis a type-conversion, not just an ordinary rewrite pattern, this information will need to be encoded in types --- we won't just be able to walk from a value to its defining op to find the pre-padding value, there just aren't values there. So I will want to add the pre-padding shape (or type) to EncodingAttr. This is a step towards that: by making EncodingAttr a data structure, it's easy then to add another field. By contrast, if it's still an enum, the combinatorics get out of hand.