Repository navigation
[SimplifyCFG] Fix illegal-width bitmap from switch lookup table - #8444
Conversation
…osoft#8421) SwitchLookupTable built bitmap APInts of width `TableSize * ResultBitWidth`, producing non-standard integer widths (i9, i17, i26, i33, i40, ...) that DXIL validation rejects - DXIL only allows i1/i8/i16/i32/i64. Round the bitmap width up to a legal width, and cap the optimization at 32 bits to avoid silently promoting to i64 (which would set the Int64Ops shader flag and add a 64-bit-integer capability requirement the source shader did not have). Wider tables fall back to the array path, or - for i1 results - preserve the original switch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Chris B (llvm-beanz)
left a comment
Chris B (llvm-beanz)
left a comment
There was a problem hiding this comment.
A few comments, but I think I'm mostly okay with this.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Joshua Batista (bob80905)
left a comment
Joshua Batista (bob80905)
left a comment
There was a problem hiding this comment.
Some questions with Alex Sepkowski (@alsepkow):
| // HLSL Change Begin: Round bitmap width up to size supported by DXIL (i16 | ||
| // or i32) |
There was a problem hiding this comment.
This is probably the least invasive way to do this for DXC and so it's fine, but in clang we'll certainly want to leave the cleanup to widen to powers of two to a legalization step.
| // HLSL Change Begin: Cap at 32 bits so the bitmap stays in a DXIL-supported | ||
| // integer width and never promotes to i64 (which would set Int64Ops). |
There was a problem hiding this comment.
There's some argument that this should be the job of DataLayout::fitsInLegalInteger itself, but that's tricky since it's technically dependent on what integer sizes are already present. Hopefully in clang we can use the byte type for this transformation, which should allow us to avoid introducing new types unintentionally.
Ashley Coleman (V-FEXrt)
left a comment
Ashley Coleman (V-FEXrt)
left a comment
There was a problem hiding this comment.
LGTM (sorry kept getting pulled away from the review)
SwitchLookupTable built bitmap APInts of width
TableSize * ResultBitWidth, producing non-standard integer widths (i9, i17, i26, i33, i40, ...) that DXIL validation rejects - DXIL only allows i1/i8/i16/i32/i64.Round the bitmap width up to a legal width, and cap the optimization at 32 bits to avoid silently promoting to i64 (which would set the Int64Ops shader flag and add a 64-bit-integer capability requirement the source shader did not have). Wider tables fall back to the array path, or - for i1 results - preserve the original switch.
Closes #8421
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com