[PATCH] D72131: [ARM][LowOverheadLoops] Update liveness info

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:577
   bool isReturnBlock() const {
-    return !empty() && back().isReturn();
+    for (auto &MI : reverse(Insts))
+      if (MI.isReturn())
----------------
This doesn't match the description above.

isReturnBlock seems to be used in a number of other places too. Would they all be OK with this change?

The one in addLiveOuts seems to mark the CSR's as live out of the block, with isn't really true. They are live out of POP the instruction. The live out's of the block are not modified. Can we modify recomputeLivenessFlags to consider isReturn instructions instead?


================
Comment at: llvm/test/CodeGen/Thumb2/LowOverheadLoops/disjoint-vcmp.mir:132
   ; CHECK:   frame-setup CFI_INSTRUCTION offset $r4, -16
-  ; CHECK:   $r7 = frame-setup tADDrSPi $sp, 2, 14, $noreg
+  ; CHECK:   dead $r7 = frame-setup tADDrSPi $sp, 2, 14, $noreg
   ; CHECK:   frame-setup CFI_INSTRUCTION def_cfa $r7, 8
----------------
samparker wrote:
> samparker wrote:
> > dmgreen wrote:
> > > Is this r7 really dead?
> > Yeah, this is a weird one, it definitely doesn't look dead. I'll look more into CFI_INSTRUCTION.
> So this looks okay to me, just because the input code labels tPUSH as killing its operands even though the CFI pseudo instructions use them. LiveRegs works on MachineInstrs, whereas CFI is some pseudo MC instruction which doesn't even contain MC operands.
Perhaps put another way, what does an instruction with a dead def and no side-effects mean? Does it mean we can remove it? What about the frame-setup flag? Does that mean we can't remove it?

The recomputeLivenessFlags seems to already be used in ifcvt. Does this already happen in the same way there? Looking at tADDrSPi, it might have hasSideEffects set, because it otherwise causes miscompiles. Which might suggest that this isn't correct, but that it's a known issue at least.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72131/new/

https://reviews.llvm.org/D72131