[Codegen] Fix correctness issues in ConvertGatherToLDS narrow type emulation#23763
Merged
lialan merged 2 commits intousers/lialan/subbyte_gather_to_ldsfrom Mar 12, 2026
Merged
Conversation
Co-authored-by: lialan <450283+lialan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] [Codegen] Emulate gather to lds for narrow element types
[Codegen] Fix correctness issues in Mar 12, 2026
ConvertGatherToLDS narrow type emulation
lialan
requested review from
Groverkss,
Max191,
krzysz00,
kuhar,
nirvedhmeshram and
qedawkins
as code owners
krzysz00
reviewed
Mar 12, 2026
|
|
||
| auto [strides, offset] = origType.getStridesAndOffset(); | ||
|
|
||
| // Fail if the offset or any stride is dynamic. |
Contributor
There was a problem hiding this comment.
... Why is this a failure?
| // Linearize: sum(idx[i] * stride[i]). | ||
| Value linearIdx = arith::ConstantIndexOp::create(rewriter, loc, 0); | ||
| // Fail if the number of indices doesn't match the rank. | ||
| if (indices.size() != strides.size()) { |
| } | ||
|
|
||
| // Linearize: offset + sum(idx[i] * stride[i]). | ||
| Value linearIdx = arith::ConstantIndexOp::create(rewriter, loc, offset); |
Contributor
There was a problem hiding this comment.
Isn't there a utility function for this these days?
This file contains hidden or 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
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.
ConvertGatherToLDShad several correctness issues inlinearizeAndPackthat could silently produce wrong IR or crash in non-assert builds.Fixes:
linearIdx = offset + sum(idx[i] * stride[i]).llvm::zip(indices, strides)silently truncated when sizes differed. Added explicitindices.size() != strides.size()guard.assert(newBits > origBits && newBits % origBits == 0)would crash in debug and silently miscompile in release. Replaced withreturn nullptr(propagated asnotifyMatchFailureby callers)."not a multiple of byte width"described the wrong invariant; corrected to"not a multiple of the new element bit width"to match the actual check (totalBits % newSrcBits != 0).origBits == newBits && 1Dfast-path that bypassed offset handling entirely.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.