[AArch64] Async unwind - Adjust unwind info in AArch64LoadStoreOptimizer

[chill]

Summary

The AArch64LoadStoreOptimnizer pass may merge a register
increment/decrement with a following memory operation. In doing so, it
may break CFI by moving a stack pointer adjustment past the CFI
instruction that described *that* adjustment.

This patch fixes this issue by moving said CFI instruction after the
merged instruction, where the SP increment/decrement actually takes
place.

[efriedma]

This could use a big comment explaining that AArch64LoadStoreOpt can run after PEI, and what that means in terms of the invariants we rely on, and the invariants we need to uphold to preserve unwind info. I can see what this patch is doing, but it's not obvious this is a comprehensive solution. Are sp adjustments the only CFI info we need to update? Does it matter if the load/store we're modifying already has a CFI directive attached to it?

[chill]

In D114547#3152164, @efriedma wrote:

This could use a big comment explaining that AArch64LoadStoreOpt can run after PEI, and what that means in terms of the invariants we rely on, and the invariants we need to uphold to preserve unwind info. I can see what this patch is doing, but it's not obvious this is a comprehensive solution. Are sp adjustments the only CFI info we need to update?

It's entirely possible this is not a comprehensive solution and I certainly wouldn't claim it was. I just have not noticed or been able to think of any other cases.

One possible thing is merging an SP update to a preceding load/store - if it jumps over a few unrelated instructions the CFA offset would be incorrect for these few instructions.
Such merging is done during epilogue emission, so CFI instructions are emitted accordingly and I couldn't steer the compiler into leaving that to load/store optimiser, where it
wold break the unwind info (and by "break" I mean a small degradation in the user experience when debugging or a slight decline in the precision of sampling profilers, certainly
nothing the affects code correctness).

Does it matter if the load/store we're modifying already has a CFI directive attached to it?

I shouldn't. It would at most create some redundancy if the load/store already wrote-back to SP and had a .cfi_def_cfa_offset, or .cfi_def_cfa immediately following it.

[efriedma]

LGTM with one minor comment

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1774

Maybe assert MI is an add or subtract, to make clear what cases you're expecting to handle here?

[chill]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1774

There are already asserts like this at lines 1794-1795 (just a little below this comment), I think they ought to do.

[efriedma]

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

1774

It's more of a documentation this; I'm a little worried someone in the future will assume this helper is more general than it actually is.

Not a big deal either way.

[chill]

Rebased

[nickdesaulniers]

Looks like this was reverted, just posting this link to Linaro's TCWG's CI failure report:
https://lore.kernel.org/llvm/[e-mail]/

[chill]

Fixed a bootstrap failure, there was a missing check for end iterator.

llvm/lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

 //===- AArch64LoadStoreOptimizer.cpp - AArch64 load/store opt. pass -------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains a pass that performs load / store related peephole
 // optimizations. This pass should be run after register allocation.
 //
+// The pass runs after the PrologEpilogInserter where we emit the CFI
+// instructions. In order to preserve the correctness of the unwind informaiton,
+// the pass should not change the order of any two instructions, one of which
+// has the FrameSetup/FrameDestroy flag or, alternatively, apply an add-hoc fix
+// to unwind information.
+//
 //===----------------------------------------------------------------------===//
 
 #include "AArch64InstrInfo.h"
 #include "AArch64MachineFunctionInfo.h"
 #include "AArch64Subtarget.h"
 #include "MCTargetDesc/AArch64AddressingModes.h"
 #include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/IR/DebugLoc.h"
 #include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCDwarf.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/DebugCounter.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cassert>
for (unsigned Count = 0; MBBI != E && Count < Limit;
 
     // Update list of instructions that read/write memory.
     if (MI.mayLoadOrStore())
       MemInsns.push_back(&MI);
   }
   return E;
 }
 
+static MachineBasicBlock::iterator
+maybeMoveCFI(MachineInstr &MI, MachineBasicBlock::iterator MaybeCFI) {
+  auto End = MI.getParent()->end();
+  if (MaybeCFI == End ||
+      MaybeCFI->getOpcode() != TargetOpcode::CFI_INSTRUCTION ||
+      !(MI.getFlag(MachineInstr::FrameSetup) ||
+        MI.getFlag(MachineInstr::FrameDestroy)) ||
+      getLdStBaseOp(MI).getReg() != AArch64::SP)
+    return End;
+
+  const MachineFunction &MF = *MI.getParent()->getParent();
+  unsigned CFIIndex = MaybeCFI->getOperand(0).getCFIIndex();
+  const MCCFIInstruction &CFI = MF.getFrameInstructions()[CFIIndex];
+  switch (CFI.getOperation()) {
+  case MCCFIInstruction::OpDefCfa:
+  case MCCFIInstruction::OpDefCfaOffset:
+    return MaybeCFI;
+  default:
+    return End;
+  }
+}
+
 MachineBasicBlock::iterator
 AArch64LoadStoreOpt::mergeUpdateInsn(MachineBasicBlock::iterator I,
                                      MachineBasicBlock::iterator Update,
                                      bool IsPreIdx) {
   assert((Update->getOpcode() == AArch64::ADDXri ||
           Update->getOpcode() == AArch64::SUBXri) &&
          "Unexpected base register update instruction to merge!");
   MachineBasicBlock::iterator E = I->getParent()->end();
   MachineBasicBlock::iterator NextI = next_nodbg(I, E);
+
+  // If updating the SP and the following instruction is CFA offset related CFI
+  // instruction move it after the merged instruction.
+  MachineBasicBlock::iterator CFI =
+      IsPreIdx ? maybeMoveCFI(*Update, next_nodbg(Update, E)) : E;
+
   // Return the instruction following the merged instruction, which is
   // the instruction following our unmerged load. Unless that's the add/sub
   // instruction we're merging, in which case it's the one after that.
   if (NextI == Update)
     NextI = next_nodbg(NextI, E);
 
   int Value = Update->getOperand(2).getImm();
   assert(AArch64_AM::getShiftValue(Update->getOperand(3).getImm()) == 0 &&
MIB = BuildMI(*I->getParent(), I, I->getDebugLoc(), TII->get(NewOpc))
               .add(getLdStRegOp(*Update))
               .add(getLdStRegOp(*I, 0))
               .add(getLdStRegOp(*I, 1))
               .add(getLdStBaseOp(*I))
               .addImm(Value / Scale)
               .setMemRefs(I->memoperands())
               .setMIFlags(I->mergeFlagsWith(*Update));
   }
-  (void)MIB;
+  if (CFI != E) {
+    MachineBasicBlock *MBB = I->getParent();
+    MBB->splice(std::next(MIB.getInstr()->getIterator()), MBB, CFI);
+  }
 
   if (IsPreIdx) {
     ++NumPreFolded;
     LLVM_DEBUG(dbgs() << "Creating pre-indexed load/store.");
   } else {
     ++NumPostFolded;
     LLVM_DEBUG(dbgs() << "Creating post-indexed load/store.");
   }

llvm/test/CodeGen/AArch64/arm64-fp128.ll

; CHECK-NEXT:    ret
   store volatile fp128 %fromdouble, fp128* @lhs, align 16
 
   ret void
 }
 
 define fp128 @test_neg(fp128 %in) {
 ; CHECK-LABEL: test_neg:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NEXT:    str q0, [sp, #-16]!
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NEXT:    ldrb w8, [sp, #15]
 ; CHECK-NEXT:    eor w8, w8, #0x80
 ; CHECK-NEXT:    strb w8, [sp, #15]
 ; CHECK-NEXT:    ldr q0, [sp], #16
 ; CHECK-NEXT:    ret
 
 ;; We convert this to fneg, and target-independent code expands it with
 ;; integer operations.
   %ret = fsub fp128 0xL00000000000000008000000000000000, %in
   ret fp128 %ret
 
 }

llvm/test/CodeGen/AArch64/fcopysign.ll

 
 @val_float = dso_local global float zeroinitializer, align 4
 @val_double = dso_local global double zeroinitializer, align 8
 @val_fp128 = dso_local global fp128 zeroinitializer, align 16
 
 define fp128 @copysign0() {
 ; CHECK-LABEL: copysign0:
 ; CHECK:       // %bb.0: // %entry
-; CHECK-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NEXT:    adrp x8, .LCPI0_0
 ; CHECK-NEXT:    ldr q0, [x8, :lo12:.LCPI0_0]
 ; CHECK-NEXT:    adrp x8, val_double
 ; CHECK-NEXT:    str q0, [sp, #-16]!
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NEXT:    ldr x8, [x8, :lo12:val_double]
 ; CHECK-NEXT:    ldrb w9, [sp, #15]
 ; CHECK-NEXT:    and x8, x8, #0x8000000000000000
 ; CHECK-NEXT:    lsr x8, x8, #56
 ; CHECK-NEXT:    bfxil w8, w9, #0, #7
 ; CHECK-NEXT:    strb w8, [sp, #15]
 ; CHECK-NEXT:    ldr q0, [sp], #16
 ; CHECK-NEXT:    ret
 ;
 ; CHECK-NONEON-LABEL: copysign0:
 ; CHECK-NONEON:       // %bb.0: // %entry
-; CHECK-NONEON-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NONEON-NEXT:    adrp x8, .LCPI0_0
 ; CHECK-NONEON-NEXT:    ldr q0, [x8, :lo12:.LCPI0_0]
 ; CHECK-NONEON-NEXT:    adrp x8, val_double
 ; CHECK-NONEON-NEXT:    str q0, [sp, #-16]!
+; CHECK-NONEON-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NONEON-NEXT:    ldr x8, [x8, :lo12:val_double]
 ; CHECK-NONEON-NEXT:    ldrb w9, [sp, #15]
 ; CHECK-NONEON-NEXT:    and x8, x8, #0x8000000000000000
 ; CHECK-NONEON-NEXT:    lsr x8, x8, #56
 ; CHECK-NONEON-NEXT:    bfxil w8, w9, #0, #7
 ; CHECK-NONEON-NEXT:    strb w8, [sp, #15]
 ; CHECK-NONEON-NEXT:    ldr q0, [sp], #16
 ; CHECK-NONEON-NEXT:    ret
 entry:
   %v = load double, double* @val_double, align 8
   %conv = fpext double %v to fp128
   %call = tail call fp128 @llvm.copysign.f128(fp128 0xL00000000000000007FFF000000000000, fp128 %conv) #2
   ret fp128 %call
 }
 
 define fp128@copysign1() {
 ; CHECK-LABEL: copysign1:
 ; CHECK:       // %bb.0: // %entry
-; CHECK-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NEXT:    adrp x8, val_fp128
 ; CHECK-NEXT:    ldr q0, [x8, :lo12:val_fp128]
 ; CHECK-NEXT:    adrp x8, val_float
 ; CHECK-NEXT:    str q0, [sp, #-16]!
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NEXT:    ldr w8, [x8, :lo12:val_float]
 ; CHECK-NEXT:    ldrb w9, [sp, #15]
 ; CHECK-NEXT:    and w8, w8, #0x80000000
 ; CHECK-NEXT:    lsr w8, w8, #24
 ; CHECK-NEXT:    bfxil w8, w9, #0, #7
 ; CHECK-NEXT:    strb w8, [sp, #15]
 ; CHECK-NEXT:    ldr q0, [sp], #16
 ; CHECK-NEXT:    ret
 ;
 ; CHECK-NONEON-LABEL: copysign1:
 ; CHECK-NONEON:       // %bb.0: // %entry
-; CHECK-NONEON-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NONEON-NEXT:    adrp x8, val_fp128
 ; CHECK-NONEON-NEXT:    ldr q0, [x8, :lo12:val_fp128]
 ; CHECK-NONEON-NEXT:    adrp x8, val_float
 ; CHECK-NONEON-NEXT:    str q0, [sp, #-16]!
+; CHECK-NONEON-NEXT:    .cfi_def_cfa_offset 16
 ; CHECK-NONEON-NEXT:    ldr w8, [x8, :lo12:val_float]
 ; CHECK-NONEON-NEXT:    ldrb w9, [sp, #15]
 ; CHECK-NONEON-NEXT:    and w8, w8, #0x80000000
 ; CHECK-NONEON-NEXT:    lsr w8, w8, #24
 ; CHECK-NONEON-NEXT:    bfxil w8, w9, #0, #7
 ; CHECK-NONEON-NEXT:    strb w8, [sp, #15]
 ; CHECK-NONEON-NEXT:    ldr q0, [sp], #16
 ; CHECK-NONEON-NEXT:    ret

llvm/test/CodeGen/AArch64/swifttail-call.ll

 ; COMMON-LABEL: caller_to8_from0:
 
 ; Key point is that the "42" should go #16 below incoming stack
 ; pointer (we didn't have arg space to reuse).
   musttail call swifttailcc void @callee_stack8([8 x i64] undef, i64 42)
   ret void
 
 ; COMMON: str {{x[0-9]+}}, [sp, #-16]!
+; COMMON-NEXT:  .cfi_def_cfa_offset 16
 ; COMMON-NEXT: b callee_stack8
 }
 
 define swifttailcc void @caller_to8_from8([8 x i64], i64 %a) #0 {
 ; COMMON-LABEL: caller_to8_from8:
 ; COMMON-NOT: sub sp,
 
 ; Key point is that the "%a" should go where at SP on entry.

 ; CHECK: stp x20
   ;call void asm "","~{r13}"() ; We get a push r13 but why not with the call
   ; below?
   %res = call swifttailcc i8* @SwiftSelf(i8 * swiftasync %context, i8* swiftself %closure)
   ret i8* %res
 }
 
 attributes #0 = { uwtable }
 No newline at end of file

llvm/test/CodeGen/AArch64/tail-call.ll

 ; COMMON-LABEL: caller_to8_from0:
 
 ; Key point is that the "42" should go #16 below incoming stack
 ; pointer (we didn't have arg space to reuse).
   tail call fastcc void @callee_stack8([8 x i64] undef, i64 42)
   ret void
 
 ; COMMON: str {{x[0-9]+}}, [sp, #-16]!
+; COMMON-NEXT: .cfi_def_cfa_offset 16
 ; COMMON-NEXT: b callee_stack8
 }
 
 define fastcc void @caller_to8_from8([8 x i64], i64 %a) #0 {
 ; COMMON-LABEL: caller_to8_from8:
 ; COMMON-NOT: sub sp,
 
 ; Key point is that the "%a" should go where at SP on entry.
; COMMON: ret
   %res.0 = insertvalue { double, [2 x double] } undef, double %val0, 0
   %res.01 = insertvalue { double, [2 x double] } %res.0, double %val1, 1, 0
   %res.012 = insertvalue { double, [2 x double] } %res.01, double %val2, 1, 1
 
   ret { double, [2 x double] } %res.012
 }
 
 attributes #0 = { uwtable }
 No newline at end of file