[CPU] Limit unrolling factors for generic ops. #17227
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
hanhanW
added
benchmarks:x86_64
Abbreviated Benchmark Summary@ commit 778787188503d0505ada631a1b3416f81aa16971 (no previous benchmark results to compare) Data-Tiling Comparison TableClick to show
Raw Latencies
[Top 3 out of 128 results showed] No improved or regressed compilation metrics 🏖️ For more information: |
hanhanW
changed the title
bjacob
requested changes
May 1, 2024
| return false; | ||
| } | ||
|
|
||
| // Check that the two indexing maps are a permutation of each other. |
There was a problem hiding this comment.
The code seems to implement Both indexing maps are permutations, and exactly one of them is the identity. What does this comment mean by "are a permutation of each other" ?
Also, this condition could be simplified to m0.isPermutation() && m1.isPermutation() && (m0.isIdentity() ^ m1.isIdentity()).
What role is played by the separate checks for isEmpty ?
There was a problem hiding this comment.
As same as above response, the code is quite old. I can help update it either in the PR or in a follow-up. isEmpty() is the check for scalars. We don't need the specialized transpose lowering when they are scalars.
| @@ -325,21 +353,33 @@ getMinTilingSizesForEachDim(mlir::FunctionOpInterface entryPointFn, | |||
|
|
|||
| // Limit unroll factor. For now, we assume the rightmost non-one tiled | |||
| // dimension is for vectorization and any other non-one dimension is for | |||
| // unrolling. | |||
| // unrolling. The util limits the second rightmost non-one tiled dimension | |||
There was a problem hiding this comment.
I meant the lambda function, let me update the comment.
Comment on lines
292
to
293
| /// Returns true if the operation is a GenericOp implementing a supported | ||
| /// transposition. |
There was a problem hiding this comment.
I was curious what "a supported transposition" means: supported in what sense?
Looking at how this function is used, it seems to mean "is defaultMaxTransposeUnrollFactor should be used instead of defaultMaxUnrollFactor for this op".
If I get that right, I would suggest that this relatively minor nuance --- just which numeric threshold value is used in some logic, not a fundamental switch of what logic is applied to the op --- is not what a reader of this comment would guess from reading the above comment, which suggests a major difference instead ("supported" vs... "not supported" ? which sounds scary).
There was a problem hiding this comment.
This is sort of precondition for transpose op lowering. We have specialized codegen for transpose ops on x86 path. It only supports n-D transpose with single input and single output. Here I'm just moving the code from below to here, but I can update the comment and and rename it to x86TransposeLoweringPrecondition. What do you think?
bjacob
approved these changes
May 1, 2024
LLITCHEV
pushed a commit
to LLITCHEV/iree
that referenced
this pull request
Jul 30, 2024
The revision also deprecates an outdated lit test that is impacted by it. It adds the other lit test simplified from the iree-org#16993 Fixes iree-org#16993 Signed-off-by: Lubo Litchev <lubol@google.com>
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.
The revision also deprecates an outdated lit test that is impacted by it. It adds the other lit test simplified from the #16993
Fixes #16993