[PATCH] D78206: [Target][ARM] Make Low Overhead Loops coexist with VPT blocks

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:230
+      MachineInstr *Op2Def = RDA.getMIOperand(MI, MI->getOperand(2));
+      if(Op1Def == nullptr && Op2Def == nullptr)
+        return false;
----------------
I'm still pondering about this... I think this should be checking that both operands are either predicated q-regs, scalar or are loop invariant, that way should ensure that the result will be the same after the transform. This method also restricts us to instructions that are directly predicated, so it would be good to add a TODO here as well.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:823
+      if (!VCTP->getOperand(1).isIdenticalTo(MI->getOperand(1)) ||
+          !RDA.hasSameReachingDef(VCTP, MI, MI->getOperand(1).getReg().id()))
+        return false;
----------------
You don't have to call id() on the register.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1239
+  //    - b. When the block uses a vpst, and is only predicated on the vctp
+  //    - c. When the block uses a vpt and contains one or more vctp
+  // 2. VPT Blocks with uniform predicates:
----------------
Does the block have to contain a vctp? Do we check that this is true?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1299
+      }
+    } else if (Block.IsOnlyPredicatedOn(LoLoop.VCTP) && Block.isVPST()) {
+      // A vpt block starting with VPST, is only predicated upon vctp and has no
----------------
I might be viewing the nesting incorrectly here, but isn't this handled on line 1250?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1315
+  // Remove remaining secondary VCTPs
+  for (MachineInstr *VCTP : LoLoop.SecondaryVCTPs) {
+    // All VCTPs that aren't marked for removal yet should be unpredicated ones.
----------------
Surely we should have discovered all of these during the loop above?


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

https://reviews.llvm.org/D78206