LowerGEPForPrivMem: treat array-of-i8 GEPs as byte offsets - #430
Open
pvelesko wants to merge 2 commits into
Open
LowerGEPForPrivMem: treat array-of-i8 GEPs as byte offsets#430pvelesko wants to merge 2 commits into
pvelesko wants to merge 2 commits into
Conversation
A GEP whose source element type is [N x i8] carries a byte offset, but the private memory transpose treats it as an aggregate index and multiplies by N a second time, giving an N-times over-stride.
The byte-offset path in TransposeHelper::handleGEPInst only recognised a bare i8 source element type. A GEP over [N x i8] fell through to the aggregate walk, which multiplied the index by N and then handed the result to HandleAllocaSources in units of the alloca's element type, giving an N-times over-stride. Recognise a source element type whose innermost scalar is i8 as byte indexing, accumulate the GEP's byte offset across all of its indices, and convert it to a lane index. For a bare i8 source element type the accumulation reduces to the single index operand, so the emitted IR is unchanged. Fixes intel#429
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
1 participant
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.
Restore SoA promotion for byte-indexed private arrays.
LLVM canonicalizes
gep T, ptr, %iintogep [sizeof(T) x i8], ptr, %i, so on anLLVM 23+ SPIR-V toolchain essentially every private-array access reaches IGC as a
GEP whose source element type is an i8 array.
TransposeHelper::handleGEPInstconverts a byte offset to a lane index only when that type is bare i8; an
[N x i8]source element type instead falls into the aggregate walk, which multiplies the index
by N and passes the result to HandleAllocaSources in units of the alloca's element
type, producing an N-times over-stride.
Commit d73553d ("Skip SOA Promotion if alloca and GEP types mismatches") prevents
that miscompile by disabling SoA promotion whenever the array-stripped GEP source
element type does not match the alloca element type. Combined with the canonicalization
above, that guard now turns off SoA promotion for effectively every private array. This
change fixes the indexing itself, so promotion is retained instead of abandoned.
The indexing defect is still reachable on master. The added lit test
IGC/Compiler/tests/PrivateMemoryResolution/arrayof_i8_gep_byte_offset.ll fails before
this change with lane index 12 instead of 3, and passes after. That test is a
constructed IR case: a second GEP moves parentLevelInst off the byte GEP so the
size-mismatch guard passes, and a constant index avoids the dynamic-index guard. We
have not observed a real kernel hitting this path on master.
The fix recognizes a source element type whose innermost scalar is i8 as byte indexing,
accumulates the GEP's byte offset over all of its indices, and divides by the element
size. For a bare i8 source element type the accumulation reduces to the single index
operand and the emitted IR is unchanged.
check-igc and check-ocloc show no other change.
Refs #429