[InstrRef][AArch64][2/4] Recognise post-fe spills and restores

[jmorse]

Summary

An important feature of instruction-referencing LiveDebugValues is that it can track variable values that are both in registers and on the stack at the same time. However, AArch64 doesn't benefit from this behaviour unless it is able to report when an instruction is a load or store from the stack. This patch implements two TargetInstrInfo hooks to collect this information.

These two functions are effectively copy-and-pastes of the pre-frame-elimination hooks, except that they recover the frame index of the stack slot accessed by examining the memory operand attached to the instruction. Instructions are recognised by being LDR* / STR* with constant offsets from the stack pointer.

The test added takes a Value, clobbers all registers with inlineasm to force the value onto the stack, then loads and returns it. LiveDebugValues should recognise this and:

• At the DBG_INST_REF mark the value as being on the stack with DBG_VALUE
• When the stack location is overwritten with the subsequent STRXui, LiveDebugValue should recognise this and place the variable in $x0, where it was loaded to.

[aprantl]

llvm/lib/Target/AArch64/AArch64InstrInfo.h

66

What does FE stand for? Maybe explain this in the doxygen comment unless this is well known to everyone working on the AArch64 backend?

[jmorse]

Refresh this patch, as I'd uh, failed to notice the test changes that resulted from it. 100% of the changes are removing the word 'Folded" from the textual assembly comments, this happens simply because the assembly printer can now recognise some instructions as being proper spills, rather than folded spills. No actual output changes at all.

(Not sure why I missed this ._.)

[jmorse]

Hi AArch64 developers (not sure who to add as reviewers), I'd like to implement the isStoreToStackSlotPostFE / isLoadFromStackSlotPostFE target hooks for AArch64, so that post-frame-elimination code can recognise spills and restores. This is important for debug-info, as we need help to track variable values as they move on and off the stack. The implementation is almost identical to the pre-frame-elimination target hooks, although instead of filtering for loads/stores that operate on frame-indexes, I've filtered for loads/stores that have stack memory operands and operate relative to the stack pointer.

There are no codegen changes in the tests -- all the test changes in this patch are the deletion of the word "Folded" from autogenerated tests, the assembly printer puts it in there if it doesn't recognise an instruction as a "proper" spill or restore.

In terms of potential codegen changes, nothing in llvm (other than debug-info) uses these hooks, except for the "getSpillSize" / "getRestoreSize" target hooks. There's a small chance that a machine-independent optimisation might behave differently as a result of this.

llvm/lib/Target/AArch64/AArch64InstrInfo.h

66

Here it's "frame elimination", i.e. after the stack slots are allocated spaces. You're right that this is probably of interest to the aarch64 maintainers, who I'll now add as reviewers...,

[jmorse]

Add back the debug-info feature test in recognise-spill-restore.mir, accidentally dropped it in last update :(

[efriedma]

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

2190

Why does it matter if the address computation SP, as opposed to the frame pointer or a scratch register? It's a load from a stack slot either way.

[dmgreen]

I happened to be looking at these functions for a different reason yesterday. Out of interest, is there anything that really makes them target dependent? It seems like it should be able to check what it needs via the MMO's PseudoValue without needing to look at opcodes (or regs, as Eli mentioned).

[jmorse]

In D104521#2861294, @dmgreen wrote:

I happened to be looking at these functions for a different reason yesterday. Out of interest, is there anything that really makes them target dependent? It seems like it should be able to check what it needs via the MMO's PseudoValue without needing to look at opcodes (or regs, as Eli mentioned).

I believe there's no target-independent way to identify the store-source register, which is what debug-info is interested in. On AArch64 it's the 3rd operand, on X86 it's the 6th (see sibling patch D105428). Previously debug-info has been looking for a killed register and assuming that that's the store-source, however there are various scenarios where spills don't kill registers.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

2190

I guess it doesn't -- I've been focusing on scenarios that involve spills and restores only, rather than general stack stores and loads. I'll revise this to not include the SP check.

[dmgreen]

In D104521#2862075, @jmorse wrote:

In D104521#2861294, @dmgreen wrote:

I happened to be looking at these functions for a different reason yesterday. Out of interest, is there anything that really makes them target dependent? It seems like it should be able to check what it needs via the MMO's PseudoValue without needing to look at opcodes (or regs, as Eli mentioned).

I believe there's no target-independent way to identify the store-source register, which is what debug-info is interested in. On AArch64 it's the 3rd operand, on X86 it's the 6th (see sibling patch D105428). Previously debug-info has been looking for a killed register and assuming that that's the store-source, however there are various scenarios where spills don't kill registers.

Oh you are returning the register. None of the other implementations of isStoreToStackSlotPostFE seem to do that, they return 1/0 or true/false. It doesn't appear to be documented anywhere what it should be returning, other than the docs for isStoreToStackSlot. It might be worth shoring that up a bit. Either deciding that it returns a register and altering the existing implementations to match, or deciding it's a boolean (which might remove the need for a target method altogether, depending on whether checking the MMO is OK).

llvm/lib/Target/AArch64/AArch64InstrInfo.h

public:
   areMemAccessesTriviallyDisjoint(const MachineInstr &MIa,
                                   const MachineInstr &MIb) const override;
 
   unsigned isLoadFromStackSlot(const MachineInstr &MI,
                                int &FrameIndex) const override;
   unsigned isStoreToStackSlot(const MachineInstr &MI,
                               int &FrameIndex) const override;
 
+  unsigned isStoreToStackSlotPostFE(const MachineInstr &MI, int &FrameIndex) const override;
+  unsigned isLoadFromStackSlotPostFE(const MachineInstr &MI, int &FrameIndex) const override;
+
   /// Does this instruction set its full destination register to zero?
   static bool isGPRZero(const MachineInstr &MI);
 
   /// Does this instruction rename a GPR without modifying bits?
   static bool isGPRCopy(const MachineInstr &MI);
 
   /// Does this instruction rename an FPR without modifying bits?
   static bool isFPRCopy(const MachineInstr &MI);

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

if (MI.getOperand(0).getSubReg() == 0 && MI.getOperand(1).isFI() &&
       FrameIndex = MI.getOperand(1).getIndex();
       return MI.getOperand(0).getReg();
     }
     break;
   }
   return 0;
 }
 
+unsigned AArch64InstrInfo::isLoadFromStackSlotPostFE(const MachineInstr &MI,
+                                               int &FrameIndex) const {
+  if (!MI.hasOneMemOperand())
+    return 0;
+
+  auto MMOI = MI.memoperands_begin();
+  const PseudoSourceValue *PVal = (*MMOI)->getPseudoValue();
+  if (!PVal)
+    return 0;
+  if (PVal->kind() != PseudoSourceValue::FixedStack)
+    return 0;
+  FrameIndex = cast<FixedStackPseudoSourceValue>(PVal)->getFrameIndex();
+
+  switch (MI.getOpcode()) {
+  default:
+    break;
+  case AArch64::LDRWui:
+  case AArch64::LDRXui:
+  case AArch64::LDRBui:
+  case AArch64::LDRHui:
+  case AArch64::LDRSui:
+  case AArch64::LDRDui:
+  case AArch64::LDRQui:
+    if (MI.getOperand(1).isReg() && MI.getOperand(2).isImm() &&
+        (MI.getOperand(1).getReg() == AArch64::SP ||
+         MI.getOperand(1).getReg() == AArch64::WSP))
+      return MI.getOperand(0).getReg();
+    break;
+  }
+
+  return 0;
+}
+
+unsigned AArch64InstrInfo::isStoreToStackSlotPostFE(const MachineInstr &MI,
+                                                    int &FrameIndex) const {
+  if (!MI.hasOneMemOperand())
+    return 0;
+
+  auto MMOI = MI.memoperands_begin();
+  const PseudoSourceValue *PVal = (*MMOI)->getPseudoValue();
+  if (!PVal)
+    return 0;
+  if (PVal->kind() != PseudoSourceValue::FixedStack)
+    return 0;
+  FrameIndex = cast<FixedStackPseudoSourceValue>(PVal)->getFrameIndex();
+
+  switch (MI.getOpcode()) {
+  default:
+    break;
+  case AArch64::STRWui:
+  case AArch64::STRXui:
+  case AArch64::STRBui:
+  case AArch64::STRHui:
+  case AArch64::STRSui:
+  case AArch64::STRDui:
+  case AArch64::STRQui:
+  case AArch64::LDR_PXI:
+  case AArch64::STR_PXI:
+    if (MI.getOperand(1).isReg() && MI.getOperand(2).isImm() &&
+        (MI.getOperand(1).getReg() == AArch64::SP ||
+         MI.getOperand(1).getReg() == AArch64::WSP))
+      return MI.getOperand(0).getReg();
+    break;
+  }
+  return 0;
+}
+
 /// Check all MachineMemOperands for a hint to suppress pairing.
 bool AArch64InstrInfo::isLdStPairSuppressed(const MachineInstr &MI) {
   return llvm::any_of(MI.memoperands(), [](MachineMemOperand *MMO) {
     return MMO->getFlags() & MOSuppressPair;
   });
 }
 
 /// Set a flag on the first MachineMemOperand to suppress pairing.

llvm/test/DebugInfo/MIR/AArch64/recognise-spill-restore.mir

+# RUN: llc %s -run-pass=livedebugvalues -march=aarch64 -o - \
+# RUN:     -experimental-debug-variable-locations | FileCheck %s
+#
+# Test that a spill then restore of a variable is tracked by LiveDebugValues on
+# AArch64. First that the value being written to the stack is recognised as
+# being on the stack, then that the subsequent load is recognised.
+#
+# CHECK: DBG_VALUE $sp, 0, {{.*}} !DIExpression(DW_OP_plus_uconst, 8),
+# CHECK: DBG_VALUE $x0, $noreg
+--- |
+  ; ModuleID = 'test.ll'
+  source_filename = "test.ll"
+  target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+  
+  define i64 @test(i64 %foo) !dbg !7 {
+    %bar = add i64 %foo, 1, !dbg !12
+    tail call void asm sideeffect "", "~{x0},~{x1},~{x2},~{x3},~{x4},~{x5},~{x6},~{x7},~{x8},~{x9},~{x10},~{x11},~{x12},~{x13},~{x14},~{x15},~{x16},~{x17},~{x18},~{x19},~{x20},~{x21},~{x22},~{x23},~{x24},~{x25},~{x26},~{x27},~{x28},~{fp},~{lr}"(), !dbg !12, !srcloc !13
+    call void @llvm.dbg.value(metadata i64 %bar, metadata !14, metadata !DIExpression()), !dbg !12
+    ret i64 %bar, !dbg !12
+  }
+  
+  ; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+  
+  attributes #0 = { nofree nosync nounwind readnone speculatable willreturn }
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5}
+  !llvm.ident = !{!6}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "test.c", directory: "/tmp/out.c")
+  !2 = !{}
+  !3 = !{i32 7, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = !{!""}
+  !7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{!10, !11, !11}
+  !10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !11 = !DIBasicType(name: "long int", size: 64, encoding: DW_ATE_signed)
+  !12 = !DILocation(line: 0, scope: !7)
+  !13 = !{i32 1}
+  !14 = !DILocalVariable(name: "bar", arg: 1, scope: !7, file: !1, line: 3, type: !11)
+
+...
+---
+name:            test
+alignment:       4
+tracksRegLiveness: true
+liveins:
+  - { reg: '$x0' }
+frameInfo:
+  stackSize:       112
+  maxAlignment:    8
+  maxCallFrameSize: 0
+stack:
+  - { id: 0, type: spill-slot, offset: -104, size: 8, alignment: 8 }
+  - { id: 1, type: spill-slot, offset: -8, size: 8, alignment: 8, callee-saved-register: '$x19' }
+  - { id: 2, type: spill-slot, offset: -16, size: 8, alignment: 8, callee-saved-register: '$x20' }
+  - { id: 3, type: spill-slot, offset: -24, size: 8, alignment: 8, callee-saved-register: '$x21' }
+  - { id: 4, type: spill-slot, offset: -32, size: 8, alignment: 8, callee-saved-register: '$x22' }
+  - { id: 5, type: spill-slot, offset: -40, size: 8, alignment: 8, callee-saved-register: '$x23' }
+  - { id: 6, type: spill-slot, offset: -48, size: 8, alignment: 8, callee-saved-register: '$x24' }
+  - { id: 7, type: spill-slot, offset: -56, size: 8, alignment: 8, callee-saved-register: '$x25' }
+  - { id: 8, type: spill-slot, offset: -64, size: 8, alignment: 8, callee-saved-register: '$x26' }
+  - { id: 9, type: spill-slot, offset: -72, size: 8, alignment: 8, callee-saved-register: '$x27' }
+  - { id: 10, type: spill-slot, offset: -80, size: 8, alignment: 8, callee-saved-register: '$x28' }
+  - { id: 11, type: spill-slot, offset: -88, size: 8, alignment: 8, callee-saved-register: '$lr' }
+  - { id: 12, type: spill-slot, offset: -96, size: 8, alignment: 8, callee-saved-register: '$fp' }
+machineFunctionInfo:
+  hasRedZone:      false
+body:             |
+  bb.0 (%ir-block.0):
+    liveins: $x0, $lr, $fp, $x27, $x28, $x25, $x26, $x23, $x24, $x21, $x22, $x19, $x20
+  
+    $sp = frame-setup SUBXri $sp, 112, 0
+    frame-setup STPXi killed $fp, killed $lr, $sp, 2 :: (store 8 into %stack.12), (store 8 into %stack.11)
+    frame-setup STPXi killed $x28, killed $x27, $sp, 4 :: (store 8 into %stack.10), (store 8 into %stack.9)
+    frame-setup STPXi killed $x26, killed $x25, $sp, 6 :: (store 8 into %stack.8), (store 8 into %stack.7)
+    frame-setup STPXi killed $x24, killed $x23, $sp, 8 :: (store 8 into %stack.6), (store 8 into %stack.5)
+    frame-setup STPXi killed $x22, killed $x21, $sp, 10 :: (store 8 into %stack.4), (store 8 into %stack.3)
+    frame-setup STPXi killed $x20, killed $x19, $sp, 12 :: (store 8 into %stack.2), (store 8 into %stack.1)
+    frame-setup CFI_INSTRUCTION def_cfa_offset 112
+    frame-setup CFI_INSTRUCTION offset $w19, -8, debug-location !12
+    frame-setup CFI_INSTRUCTION offset $w20, -16, debug-location !12
+    frame-setup CFI_INSTRUCTION offset $w21, -24, debug-location !12
+    frame-setup CFI_INSTRUCTION offset $w22, -32, debug-location !12
+    frame-setup CFI_INSTRUCTION offset $w23, -40, debug-location !12
+    frame-setup CFI_INSTRUCTION offset $w24, -48, debug-location !12
+    frame-setup CFI_INSTRUCTION offset $w25, -56, debug-location !12
+    frame-setup CFI_INSTRUCTION offset $w26, -64, debug-location !12
+    frame-setup CFI_INSTRUCTION offset $w27, -72, debug-location !12
+    frame-setup CFI_INSTRUCTION offset $w28, -80, debug-location !12
+    frame-setup CFI_INSTRUCTION offset $w30, -88, debug-location !12
+    frame-setup CFI_INSTRUCTION offset $w29, -96, debug-location !12
+    renamable $x8 = ADDXri killed renamable $x0, 1, 0, debug-instr-number 1, debug-location !12
+    STRXui killed renamable $x8, $sp, 1 :: (store 8 into %stack.0)
+    INLINEASM &"", 1 /* sideeffect attdialect */, 12 /* clobber */, implicit-def dead early-clobber $x0, 12 /* clobber */, implicit-def dead early-clobber $x1, 12 /* clobber */, implicit-def dead early-clobber $x2, 12 /* clobber */, implicit-def dead early-clobber $x3, 12 /* clobber */, implicit-def dead early-clobber $x4, 12 /* clobber */, implicit-def dead early-clobber $x5, 12 /* clobber */, implicit-def dead early-clobber $x6, 12 /* clobber */, implicit-def dead early-clobber $x7, 12 /* clobber */, implicit-def dead early-clobber $x8, 12 /* clobber */, implicit-def dead early-clobber $x9, 12 /* clobber */, implicit-def dead early-clobber $x10, 12 /* clobber */, implicit-def dead early-clobber $x11, 12 /* clobber */, implicit-def dead early-clobber $x12, 12 /* clobber */, implicit-def dead early-clobber $x13, 12 /* clobber */, implicit-def dead early-clobber $x14, 12 /* clobber */, implicit-def dead early-clobber $x15, 12 /* clobber */, implicit-def dead early-clobber $x16, 12 /* clobber */, implicit-def dead early-clobber $x17, 12 /* clobber */, implicit-def dead early-clobber $x18, 12 /* clobber */, implicit-def dead early-clobber $x19, 12 /* clobber */, implicit-def dead early-clobber $x20, 12 /* clobber */, implicit-def dead early-clobber $x21, 12 /* clobber */, implicit-def dead early-clobber $x22, 12 /* clobber */, implicit-def dead early-clobber $x23, 12 /* clobber */, implicit-def dead early-clobber $x24, 12 /* clobber */, implicit-def dead early-clobber $x25, 12 /* clobber */, implicit-def dead early-clobber $x26, 12 /* clobber */, implicit-def dead early-clobber $x27, 12 /* clobber */, implicit-def dead early-clobber $x28, 12 /* clobber */, implicit-def dead early-clobber $fp, 12 /* clobber */, implicit-def dead early-clobber $lr, !13, debug-location !12
+    DBG_INSTR_REF 1, 0, !14, !DIExpression(), debug-location !12
+    $x0 = LDRXui $sp, 1 :: (load 8 from %stack.0)
+    ; This store added to evict the variable value from the stack, forces
+    ; LiveDebugValues to relocate it to $x0
+    STRXui $xzr, $sp, 1 :: (store 8 into %stack.0)
+    $x20, $x19 = frame-destroy LDPXi $sp, 12, debug-location !12 :: (load 8 from %stack.2), (load 8 from %stack.1)
+    $x22, $x21 = frame-destroy LDPXi $sp, 10, debug-location !12 :: (load 8 from %stack.4), (load 8 from %stack.3)
+    $x24, $x23 = frame-destroy LDPXi $sp, 8, debug-location !12 :: (load 8 from %stack.6), (load 8 from %stack.5)
+    $x26, $x25 = frame-destroy LDPXi $sp, 6, debug-location !12 :: (load 8 from %stack.8), (load 8 from %stack.7)
+    $x28, $x27 = frame-destroy LDPXi $sp, 4, debug-location !12 :: (load 8 from %stack.10), (load 8 from %stack.9)
+    $fp, $lr = frame-destroy LDPXi $sp, 2, debug-location !12 :: (load 8 from %stack.12), (load 8 from %stack.11)
+    $sp = frame-destroy ADDXri $sp, 112, 0, debug-location !12
+    RET undef $lr, implicit killed $x0, debug-location !12
+
+...