[PATCH] D72713: [ARM][LowOverheadLoops] Check loop liveouts

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Just a few nits/questions inlined.



================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:636
+  // not necessarily match the semantics of the uses in the exit blocks.
+  const TargetRegisterClass *QPRs = TRI.getRegClass(ARM::MQPRRegClassID);
+  SmallVector<MCPhysReg, 2> LiveOuts;
----------------
Nit: perhaps good to mention here for completeness that MVE instructions that e.g. accumulate in GPR registers don't need checking, because tail-predication doesn't impact these values?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:641
+      continue;
+    else if (MO.isDef() && isRegLiveInExitBlocks(&ML, MO.getReg()))
+      LiveOuts.push_back(MO.getReg());
----------------
Just checking: do we have test with only live Q registers in exit blocks that are not defined in the loop? That would perhaps be a bit strange, but why not...


================
Comment at: llvm/test/CodeGen/Thumb2/LowOverheadLoops/wrong-vctp-operand-liveout.mir:189
+
+    renamable $vpr = MVE_VCTP32 killed renamable $r2, 0, $noreg
+    renamable $q0 = MVE_VPSEL killed renamable $q1, killed renamable $q0, 0, killed renamable $vpr
----------------
q0 is live out/in, so the change in this patch will prevent TP. Just curious how that is related to the use of `r2`. I mean, that makes sense, the test is good to have, but is not the result of this patch?



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

https://reviews.llvm.org/D72713