[llvm] [LLVM] [X86] Fix integer overflows in frame layout for huge frames (PR #101840)
Wesley Wiser via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 21 19:00:20 PDT 2024
wesleywiser wrote:
Looking at the repro mentioned,, I see exactly one difference in the asm between prior to my change and after my change:
```diff
.LBB0_46: # %while.body.i.preheader.i
# in Loop: Header=BB0_10 Depth=1
movl -20(%ebp), %ebx # 4-byte Reload
- leal 2147483423(%ebp), %eax
+ leal -2147483873(%ebp), %eax
movl $1, %ecx
addl $2147483647, %ebx # imm = 0x7FFFFFFF
```
If I explicitly truncate the offset back to 32-bits, then I get the same output as before my change:
```diff
@@ -988,7 +988,7 @@ X86RegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II,
assert((Is64Bit || FitsIn32Bits) &&
"Requesting 64-bit offset in 32-bit immediate!");
if (Offset != 0 || !tryOptimizeLEAtoMOV(II))
- MI.getOperand(FIOperandNum + 3).ChangeToImmediate(Offset);
+ MI.getOperand(FIOperandNum + 3).ChangeToImmediate((int64_t)((int)FIOffset + (int)Imm));
} else {
// Offset is symbolic. This is extremely rare.
uint64_t Offset = FIOffset +
```
So I think the actual issue here is that the assertion was [previously written incorrectly](https://github.com/llvm/llvm-project/pull/101840/files#diff-9634de34ebe6c0515879267e2eddfa69a67d18de32f7767c24c09588e85f3654L960-L962) and didn't fire when it should have.
I guess one possible "fix" here is to just remove the assert. Alternatively, we could truncate the offset back to 32-bit when targeting x86 like we used to.
Does that seem reasonable? Suggestions? 🙂
https://github.com/llvm/llvm-project/pull/101840
More information about the llvm-commits
mailing list