[AArch64] Fix offset calculation

[smeenai]

Summary

r374772 changed Offset to be an int64_t but left NewOffset as an int,
which led to integer promotion issues in this calculation and resulted
in bad offset values. Promote NewOffset to int64_t as well to fix this,
and promote EmittableOffset as well, since its one user passes it to a
function which takes an int64_t anyway. This manifested as an
out-of-memory when building the Swift standard library for Android
aarch64. Test case suggested by Sander de Smalen!

[smeenai]

I have no idea how to write a test for this; I'm completely unfamiliar with backends. I have a Swift compilation command that reproduces the OOM mentioned in the commit message, and I think I should be able to get IR or MIR from that, but I'd appreciate guidance crafting a test case if it's considered necessary for this commit.

[smeenai]

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

3442

EmittableOffset is still an int ... idk if I should be promoting that too.

[smeenai]

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

3438

Just casting NewOffset to int64_t over here is actually sufficient, but I figured changing the type entirely was better/cleaner.

[sdesmalen]

Thanks for fixing this @smeenai!

In D69018#1710443, @smeenai wrote:

I have no idea how to write a test for this; I'm completely unfamiliar with backends. I have a Swift compilation command that reproduces the OOM mentioned in the commit message, and I think I should be able to get IR or MIR from that, but I'd appreciate guidance crafting a test case if it's considered necessary for this commit.

You can create a MIR test with a single instruction accessing a pre-allocated stackslot with a very large offset, and check that the offset is generated correctly.

e.g. the following MIR

---
name: D69018
tracksRegLiveness: true
fixedStack:
  - { id: 0, offset: 2147483648, size: 1}
body: |
  bb.0:
    $x0 = LDRXui %fixed-stack.0, 0
    RET_ReallyLR
...

compiles with your patch, but fails to complete (i.e. it keeps running) without it.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

3442

That should be fine, because it will be used for the immediate field of an instruction.

[smeenai]

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

3442

I decided to change it to int64_t as well, since the only call site which uses this parameter passes it to MachineOperand::ChangeToImmediate, which takes an int64_t parameter.

[smeenai]

Add test

[smeenai]

In D69018#1711129, @sdesmalen wrote:

Thanks for fixing this @smeenai!

In D69018#1710443, @smeenai wrote:

I have no idea how to write a test for this; I'm completely unfamiliar with backends. I have a Swift compilation command that reproduces the OOM mentioned in the commit message, and I think I should be able to get IR or MIR from that, but I'd appreciate guidance crafting a test case if it's considered necessary for this commit.

You can create a MIR test with a single instruction accessing a pre-allocated stackslot with a very large offset, and check that the offset is generated correctly.

e.g. the following MIR

---
name: D69018
tracksRegLiveness: true
fixedStack:
  - { id: 0, offset: 2147483648, size: 1}
body: |
  bb.0:
    $x0 = LDRXui %fixed-stack.0, 0
    RET_ReallyLR
...

compiles with your patch, but fails to complete (i.e. it keeps running) without it.

Thanks for the test case! I added it.

[sdesmalen]

llvm/test/CodeGen/AArch64/framelayout-large-offset.mir

2

nit: I would probably add -run-pass=prologepilog, to reduce possible interference from other passes.

15

Sorry I didn't notice this earlier when I posted the example MIR, but these subs are wrong. The offset is at a positive distance from the SP, so it should use add here. If I change the offset from 2147483648 to 2147483632 that changes. So I expect some other changes are needed to fix this.

[smeenai]

llvm/test/CodeGen/AArch64/framelayout-large-offset.mir

15

If I revert my change and r374772, the problem still persists, so that seems like a pre-existing issue?

[smeenai]

Use -run-pass=prologepilog in test

[smeenai]

llvm/test/CodeGen/AArch64/framelayout-large-offset.mir

15

Given that this is a pre-existing problem, would you be okay with submitting this to solve the OOM and then filing a bug for the incorrect offset calculation?

[thegameg]

llvm/test/CodeGen/AArch64/framelayout-large-offset.mir

2

I believe this is missing a | FileCheck %s to perform the actual checks.

[sdesmalen]

llvm/test/CodeGen/AArch64/framelayout-large-offset.mir

15

Sigh, I'm clearly having a bit of a senior moment here :) maxint is 2147483647 (forgot the -1 in my calculator). But with the fixed/updated offset, the result is the same with/without your patch, so we'll need something different to test it.

[smeenai]

Fix test

[smeenai]

llvm/test/CodeGen/AArch64/framelayout-large-offset.mir

2

Oops, forgot to add that back after testing something. Thanks!

15

The test as it stands does infinite loop/OOM without my patch though and completes successfully after.

[smeenai]

Update test

[smeenai]

@sdesmalen How does the new test case look?

[sdesmalen]

In D69018#1711613, @smeenai wrote:

@sdesmalen How does the new test case look?

Thanks, the new test-case seems to cover the case well. It is out of range of the immediate and with NewOffset as int64_t the expression NewOffset * Scale should no longer be evaluated as unsigned.

LGTM

llvm/test/CodeGen/AArch64/framelayout-large-offset.mir

3

nit: You'll want to change the name of the test/function now.

[smeenai]

Thanks for the quick review!

llvm/test/CodeGen/AArch64/framelayout-large-offset.mir

3

Yup, will change before committing.

[nickdesaulniers]

Likely also fixes regressions for aarch64 linux kernel builds: https://travis-ci.com/ClangBuiltLinux/continuous-integration/jobs/246197698

llvm/lib/Target/AArch64/AArch64InstrInfo.h

 /// If set, @p OutUseUnscaledOp will contain the whether @p MI should be
 /// turned into an unscaled operator, which opcode is in @p OutUnscaledOp.
 /// If set, @p EmittableOffset contains the amount that can be set in @p MI
 /// (possibly with @p OutUnscaledOp if OutUseUnscaledOp is true) and that
 /// is a legal offset.
 int isAArch64FrameOffsetLegal(const MachineInstr &MI, StackOffset &Offset,
                               bool *OutUseUnscaledOp = nullptr,
                               unsigned *OutUnscaledOp = nullptr,
-                              int *EmittableOffset = nullptr);
+                              int64_t *EmittableOffset = nullptr);
 
 static inline bool isUncondBranchOpcode(int Opc) { return Opc == AArch64::B; }
 
 static inline bool isCondBranchOpcode(int Opc) {
   switch (Opc) {
   case AArch64::Bcc:
   case AArch64::CBZW:
   case AArch64::CBZX:

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

default:
     return false;
   }
 }
 
 int llvm::isAArch64FrameOffsetLegal(const MachineInstr &MI,
                                     StackOffset &SOffset,
                                     bool *OutUseUnscaledOp,
                                     unsigned *OutUnscaledOp,
-                                    int *EmittableOffset) {
+                                    int64_t *EmittableOffset) {
   // Set output values in case of early exit.
   if (EmittableOffset)
     *EmittableOffset = 0;
   if (OutUseUnscaledOp)
     *OutUseUnscaledOp = false;
   if (OutUnscaledOp)
     *OutUnscaledOp = 0;
 
if (useUnscaledOp &&
       !AArch64InstrInfo::getMemOpInfo(*UnscaledOp, Scale, Width, MinOff, MaxOff))
     llvm_unreachable("unhandled opcode in isAArch64FrameOffsetLegal");
 
   int64_t Remainder = Offset % Scale;
   assert(!(Remainder && useUnscaledOp) &&
          "Cannot have remainder when using unscaled op");
 
   assert(MinOff < MaxOff && "Unexpected Min/Max offsets");
-  int NewOffset = Offset / Scale;
+  int64_t NewOffset = Offset / Scale;
   if (MinOff <= NewOffset && NewOffset <= MaxOff)
     Offset = Remainder;
   else {
     NewOffset = NewOffset < 0 ? MinOff : MaxOff;
     Offset = Offset - NewOffset * Scale + Remainder;
   }
 
   if (EmittableOffset)
     *EmittableOffset = NewOffset;
   if (OutUseUnscaledOp)
     *OutUseUnscaledOp = useUnscaledOp;
   if (OutUnscaledOp && UnscaledOp)
     *OutUnscaledOp = *UnscaledOp;
 
   if (IsMulVL)
     SOffset = StackOffset(Offset, MVT::nxv1i8) +
               StackOffset(SOffset.getBytes(), MVT::i8);
if (Opcode == AArch64::ADDSXri || Opcode == AArch64::ADDXri) {
     emitFrameOffset(*MI.getParent(), MI, MI.getDebugLoc(),
                     MI.getOperand(0).getReg(), FrameReg, Offset, TII,
                     MachineInstr::NoFlags, (Opcode == AArch64::ADDSXri));
     MI.eraseFromParent();
     Offset = StackOffset();
     return true;
   }
 
-  int NewOffset;
+  int64_t NewOffset;
   unsigned UnscaledOp;
   bool UseUnscaledOp;
   int Status = isAArch64FrameOffsetLegal(MI, Offset, &UseUnscaledOp,
                                          &UnscaledOp, &NewOffset);
   if (Status & AArch64FrameOffsetCanUpdate) {
     if (Status & AArch64FrameOffsetIsLegal)
       // Replace the FrameIndex with FrameReg.
       MI.getOperand(FrameRegIdx).ChangeToRegister(FrameReg, false);

llvm/test/CodeGen/AArch64/framelayout-large-offset.mir

+# RUN: llc -mtriple=aarch64-none-linux-gnu -run-pass=prologepilog %s -o - | FileCheck %s
+---
+name: framelayout_large_offset
+tracksRegLiveness: true
+fixedStack:
+  - { id: 0, offset: 0, size: 1}
+body: |
+  bb.0:
+    $x0 = LDURXi %fixed-stack.0, -264
+    RET_ReallyLR
+...
+# CHECK:      name:            framelayout_large_offset
+# CHECK:      body:             |
+# CHECK-NEXT:   bb.0:
+# CHECK-NEXT:     $x8 = SUBXri $sp, 8, 0
+# CHECK-NEXT:     $x0 = LDURXi killed $x8, -256
+# CHECK-NEXT:     RET_ReallyLR
+