[PATCH] D27559: AArch64: Enable post-ra liveness updates

[Matthias Braun via Phabricator via llvm-commits]

MatzeB marked 5 inline comments as done.
MatzeB added a comment.

Thanks for the review!



================
Comment at: lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:901
+    // 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 LSR
+    // handling ensures the register value is restored before RET, but we have
----------------
gberry wrote:
> Typo? "LSR" -> "LR"
Yes a typo, but I wanted to write CSR (Callee Saved Register)


================
Comment at: lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:902
+    // function and missing live-ins. We are fine in practice because LSR
+    // handling ensures the register value is restored before RET, but we have
+    // need the undef flag here to appease the MachineVerifier liveness checks.
----------------
gberry wrote:
> Typo: remove "have"
yep


================
Comment at: lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:906
         BuildMI(MBB, MBBI, MI.getDebugLoc(), TII->get(AArch64::RET))
-          .addReg(AArch64::LR);
+          .addReg(AArch64::LR, RegState::Undef);
     transferImpOps(MI, MIB, MIB);
----------------
gberry wrote:
> 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?
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).


================
Comment at: lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp:835
   }
+  StoreI->clearRegisterKills(StRt, TRI);
+
----------------
gberry wrote:
> This is conservative, right?  Ideally you would copy the kill flags over to the last new instruction that uses StRt?
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)


================
Comment at: test/CodeGen/AArch64/loh.mir:57
     BL @extfunc, csr_aarch64_aapcs ; clobbers x9
-    %x9 = ADDXri %x9, target-flags(aarch64-pageoff) @g0, 0    
+    ; Liveness verification forces me to use 'undef' in front of %x9...
+    %x9 = ADDXri undef %x9, target-flags(aarch64-pageoff) @g0, 0
----------------
gberry wrote:
> Maybe add "because of the above clobber of x9" to this comment?
ok


================
Comment at: test/CodeGen/AArch64/loh.mir:103
     %x11 = ADRP target-flags(aarch64-page, aarch64-got) @g5
-    %s11 = LDRSui %x4, target-flags(aarch64-pageoff, aarch64-got) @g5
+    %s11 = LDRSui %x11, target-flags(aarch64-pageoff, aarch64-got) @g5
 
----------------
gberry wrote:
> I don't follow this change of registers.
Oh this should have ended up in a separate commit. It fixes a bug in the test not a problem related to the trackLivenessAfterRegAlloc().



================
Comment at: test/CodeGen/AArch64/loh.mir:173
     %x10 = ADRP target-flags(aarch64-page) @g0
-    HINT 0, implicit def dead %x10 ; clobbers x10
     %x11 = ADDXri %x10, target-flags(aarch64-pageoff) @g0, 0
----------------
gberry wrote:
> I don't follow this either.  It seems like you are changing what is being tested.
Yep another fix for a bug in the test that should have been in a separate commit.


Repository:
  rL LLVM

https://reviews.llvm.org/D27559