[SimplifyCFG] Fix illegal-width bitmap from switch lookup table#8444
[SimplifyCFG] Fix illegal-width bitmap from switch lookup table#8444damyanp wants to merge 5 commits into
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>
llvm-beanz
left a comment
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. |
| // or i32) | ||
| uint64_t RawBitMapWidth = TableSize * IT->getBitWidth(); | ||
| uint64_t BitMapWidth = | ||
| NextPowerOf2(std::max(UINT64_C(15), RawBitMapWidth - 1)); |
There was a problem hiding this comment.
nit: @alsepkow recommended this alternative:
uint64_t BitMapWidth = std::max(UINT64_C(16), PowerOf2Ceil(RawBitMapWidth));
There was a problem hiding this comment.
So that's...different...I'm not sure why it's worth changing?
There was a problem hiding this comment.
Our opinion is it's functionally equivalent but a bit simpler / more readable.
| ; | ||
| ; CHECK-LABEL: @switch_i8_5_cases | ||
| ; CHECK-NOT: i40 | ||
| ; CHECK: ret i8 |
There was a problem hiding this comment.
If there is an array being made here, can we add a check for the array?
There was a problem hiding this comment.
I also noticed that this case uses i8's that we've just taken steps to avoid in other places, so fixed that as well.
|
|
||
| end: | ||
| ; Non-linear values prevent the LinearMap fast path so the bitmap path is | ||
| ; the one that would have been chosen. |
There was a problem hiding this comment.
This seems to suggest a bitmap is being created, but such a bitmap would be > 32 bits, right? Is this comment saying that thanks to non-linear values, no bitmap is produced?
3 participants
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