AArch64: Enable post-ra liveness updates

[MatzeB]

Summary

Fix the situations in which liveness information was not properly maintained/updated.

This is a prerequesite to have the rewritten Macho CollectLOH pass work reliably.

[MatzeB]

ping

[gberry]

lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

901

Typo? "LSR" -> "LR"

902

Typo: remove "have"

907

Is the reason you need LR to be undef here because we haven't added the corresponding def of LR to the function at this point?

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

835

This is conservative, right? Ideally you would copy the kill flags over to the last new instruction that uses StRt?

test/CodeGen/AArch64/ldst-opt-dbg-limit.mir

128

I don't understand why you removed the kills here.

test/CodeGen/AArch64/loh.mir

57 ↗

(On Diff #80705)

Maybe add "because of the above clobber of x9" to this comment?

103 ↗

(On Diff #80705)

I don't follow this change of registers.

173 ↗

(On Diff #80705)

I don't follow this either. It seems like you are changing what is being tested.

[MatzeB]

Thanks for the review!

lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

901

Yes a typo, but I wanted to write CSR (Callee Saved Register)

902

yep

907

To give some context: I believe Ret_ReallyLR is hack to hide the fact that LR is used from the register allocator, because in old versions of the code simply reading a register would trigger the callee saved register handling to unnecessarily spill and restore. I actually fixed that bad behavior a year ago, however trying to get rid of the Ret_ReallyLR hack in this commit put me a lot deeper into the rabid hole than I wanted to go when I tried it so I left that for another day in the end.

The reason for the undef flag is that LR now looks unused (at the end of the function), so in some instances we would produce operands like LR<kill> in the middle of the function indicating liveness analysis that the value gets killed there. This effectively is invalid liveness information but without trackLivenessAfterRegAlloc() enabled nobody would access liveness information late in the compiler so the verifier would not check it. So to appease the verifier I add an undef flag here, the callee save register handling will always ensure anyway that LR is restored at the end, no matter if the return has an actual LR use or not.
(Yes it feels like a hack but we are forced to do it as long as the Ret_ReallyLR hack is around).

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

835

Yes it is conservative. But kill flags themselfes are considered conservative nowadays anyway, because updating them correctly turned out to be a maintenance nightmare so nowadays we usually go with the simpler fix and simply remove them.
(Getting completely rid of kill flags is on the long term TODO list, but that is another longer story, parts of it can be seen in my patch series to have the regscavenger/prologepilog inserter walk backwards instead of forwards which is somewhat staled on hard to debug bot failure)

test/CodeGen/AArch64/loh.mir

57 ↗

(On Diff #80705)

ok

103 ↗

(On Diff #80705)

Oh this should have ended up in a separate commit. It fixes a bug in the test not a problem related to the trackLivenessAfterRegAlloc().

173 ↗

(On Diff #80705)

Yep another fix for a bug in the test that should have been in a separate commit.

[MatzeB]

Update as discussed. Note that the loh mir test is no longer part of this patch as it is currently reverted from ToT.

[gberry]

LGTM

lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp

case AArch64::MOVaddrEXT: {
     return true;
   }
 
   case AArch64::MOVi32imm:
     return expandMOVImm(MBB, MBBI, 32);
   case AArch64::MOVi64imm:
     return expandMOVImm(MBB, MBBI, 64);
   case AArch64::RET_ReallyLR: {
+    // Hiding the LR use with RET_ReallyLR may lead to extra kills in the
+    // function and missing live-ins. We are fine in practice because callee
+    // saved register handling ensures the register value is restored before
+    // RET, but we need the undef flag here to appease the MachineVerifier
+    // liveness checks.
     MachineInstrBuilder MIB =
         BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(AArch64::RET))
-          .addReg(AArch64::LR);
+          .addReg(AArch64::LR, RegState::Undef);
     transferImpOps(MI, MIB, MIB);
     MI.eraseFromParent();
     return true;
   }
   case AArch64::CMP_SWAP_8:
     return expandCMP_SWAP(MBB, MBBI, AArch64::LDAXRB, AArch64::STLXRB,
                           AArch64::SUBSWrx,
                           AArch64_AM::getArithExtendImm(AArch64_AM::UXTB, 0),

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp

assert((IsStoreXReg ||
           TRI->getRegClass(AArch64::GPR32RegClassID)->contains(StRt)) &&
          "Unexpected RegClass");
 
   MachineInstr *BitExtMI;
   if (LoadSize == StoreSize && (LoadSize == 4 || LoadSize == 8)) {
     // Remove the load, if the destination register of the loads is the same
     // register for stored value.
     if (StRt == LdRt && LoadSize == 8) {
+      StoreI->clearRegisterKills(StRt, TRI);
       DEBUG(dbgs() << "Remove load instruction:\n    ");
       DEBUG(LoadI->print(dbgs()));
       DEBUG(dbgs() << "\n");
       LoadI->eraseFromParent();
       return NextI;
     }
     // Replace the load with a mov if the load and store are in the same size.
     BitExtMI =
if (UnscaledLdOffset == UnscaledStOffset) {
           BuildMI(*LoadI->getParent(), LoadI, LoadI->getDebugLoc(),
                   TII->get(IsStoreXReg ? AArch64::UBFMXri : AArch64::UBFMWri),
                   DestReg)
               .addReg(StRt)
               .addImm(Immr)
               .addImm(Imms);
     }
   }
+  StoreI->clearRegisterKills(StRt, TRI);
+
   (void)BitExtMI;
 
   DEBUG(dbgs() << "Promoting load by replacing :\n    ");
   DEBUG(StoreI->print(dbgs()));
   DEBUG(dbgs() << "    ");
   DEBUG(LoadI->print(dbgs()));
   DEBUG(dbgs() << "  with instructions:\n    ");
   DEBUG(StoreI->print(dbgs()));

lib/Target/AArch64/AArch64RegisterInfo.h

public:
   bool hasBasePointer(const MachineFunction &MF) const;
   unsigned getBaseRegister() const;
 
   // Debug information queries.
   unsigned getFrameRegister(const MachineFunction &MF) const override;
 
   unsigned getRegPressureLimit(const TargetRegisterClass *RC,
                                MachineFunction &MF) const override;
+
+  bool trackLivenessAfterRegAlloc(const MachineFunction&) const override {
+    return true;
+  }
 };
 
 } // end namespace llvm
 
 #endif

test/CodeGen/AArch64/ldst-opt-dbg-limit.mir

--- |
   
   attributes #0 = { nounwind }
 
 ...
 ---
 name:            promote-load-from-store
 alignment:       2
 exposesReturnsTwice: false
-tracksRegLiveness: false
+tracksRegLiveness: true
 liveins:         
   - { reg: '%x0' }
   - { reg: '%w1' }
 frameInfo:       
   isFrameAddressTaken: false
   isReturnAddressTaken: false
   hasStackMap:     false
   hasPatchPoint:   false
   stackSize:       0
   offsetAdjustment: 0
   maxAlignment:    0
   adjustsStack:    false
   hasCalls:        false
   maxCallFrameSize: 0
   hasOpaqueSPAdjustment: false
   hasVAStart:      false
   hasMustTailInVarArgFunc: false
 body:             |
   bb.0 (%ir-block.0):
-    liveins: %w1, %x0
+    liveins: %w1, %x0, %lr
   
     STRWui killed %w1, %x0, 0 :: (store 4 into %ir.dst)
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
bb.0 (%ir-block.0):
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     %w0 = LDRHHui killed %x0, 1 :: (load 2 from %ir.dst1)
     RET %lr, implicit %w0
 
 ...
 # CHECK-LABEL: name:            promote-load-from-store
-# CHECK: STRWui killed %w1
+# CHECK: STRWui %w1
 # CHECK: UBFMWri %w1
 ---
 name:            store-pair
 alignment:       2
 exposesReturnsTwice: false
-tracksRegLiveness: false
+tracksRegLiveness: true
 liveins:         
   - { reg: '%x0' }
   - { reg: '%w1' }
 frameInfo:       
   isFrameAddressTaken: false
   isReturnAddressTaken: false
   hasStackMap:     false
   hasPatchPoint:   false
   stackSize:       0
   offsetAdjustment: 0
   maxAlignment:    0
   adjustsStack:    false
   hasCalls:        false
   maxCallFrameSize: 0
   hasOpaqueSPAdjustment: false
   hasVAStart:      false
   hasMustTailInVarArgFunc: false
 body:             |
   bb.0 (%ir-block.0):
-    liveins: %w1, %x0
+    liveins: %w1, %x0, %lr
   
     STRWui %w1, %x0, 0 :: (store 4 into %ir.dst01)
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     CFI_INSTRUCTION 0
     STRWui killed %w1, killed %x0, 1 :: (store 4 into %ir.dst1)
     RET %lr
 
 ...
 # CHECK-LABEL: name:            store-pair
 # CHECK: STPWi

test/CodeGen/AArch64/movimm-wzr.mir

body:             |
   bb.0 (%ir-block.0):
     %wzr = MOVi32imm 42
     %xzr = MOVi64imm 42
     RET_ReallyLR implicit killed %w0
 
 ...
 
 # CHECK: bb.0
-# CHECK-NEXT: RET %lr
+# CHECK-NEXT: RET undef %lr