[PATCH] D90461: [ARM][LowOverheadLoops] Merge a VCMP and the new VPST into a VPT

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1534
+        MachineInstr *VPST = Insts.front();
+        auto DivergentNext = ++MachineBasicBlock::iterator(Divergent);
+        bool DivergentNextIsPredicated =
----------------
We are sure here that Divergent will not be nullptr? So ++ on the iterator is valid?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1569
+        if (VCMP &&
+            RDA->hasSameReachingDef(VCMP, VPST, VCMP->getOperand(1).getReg()) &&
+            RDA->hasSameReachingDef(VCMP, VPST, VCMP->getOperand(2).getReg())) {
----------------
Which VPST is this? The one for the beginning of this block, or the beginning of the next block?

I think if we have a VCMP in the middle of a block we can convert it to a VPT always.
And if we have a VCMP at the end of a block, we may be able to fold it into the _next_ block if this check that the defs are the same holds.

Which looks like mostly what we have here now, but the folding should happen between the VPST of a block and a VCMP prior to it, if there is one.

Is it worth making getDivergent loop until the last but one instruction in the block, so that it doesn't return divergent if the last instruction is producing a predicate. Then we just remove the predicate as needed.


================
Comment at: llvm/test/CodeGen/Thumb2/LowOverheadLoops/vcmp-vpst-combination.ll:44
 
+define hidden i32 @vcmp_new_vpst_combination(i32 %len, i32* nocapture readonly %arr) local_unnamed_addr #0 {
+; CHECK-LABEL: vcmp_new_vpst_combination:
----------------
Can you remove hidden and  the "local_unnamed_addr #0"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90461