[PATCH] D86087: [ARM] Use mov operand if the mov cannot be moved while tail predicating

[Sam Parker via Phabricator via llvm-commits]

samparker added a comment.

I think having the IR test is a good idea, but could you also upload it as a MIR too, just because this pass is very far away from the IR input.



================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:229
     MachineInstr *VCTP = nullptr;
-    SmallPtrSet<MachineInstr*, 4> SecondaryVCTPs;
+    MachineOperand TPNumElements;
+    SmallPtrSet<MachineInstr *, 4> SecondaryVCTPs;
----------------
Maybe just store the Register instead?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:298
     // predicating.
     MachineOperand &getCount() {
+      return IsTailPredicationLegal() ? TPNumElements : Start->getOperand(0);
----------------
I know it's not your doing, but would you mind renaming this method to something a bit more descriptive?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:458
+  TPNumElements = VCTP->getOperand(1);
+  Register NumElements = TPNumElements.getReg();
 
----------------
I think just having NumElements as a member of the class would make sense,


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:489
+        // insertion point
+        if (ElemDef->getOpcode() == ARM::tMOVr &&
+            RDA.getUniqueReachingMIDef(ElemDef,
----------------
In ARMBaseInstrInfo.h we have isMovRegOpcode, which also covers the T2 version.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:494
+                                           ElemDef->getOperand(1).getReg())) {
+          TPNumElements = ElemDef->getOperand(1);
+          NumElements = TPNumElements.getReg();
----------------
Maybe only call ElemDef->getOperand(1) once and give the variable a nice name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86087