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

[Sam Tebbs via Phabricator via llvm-commits]

samtebbs added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1534
+        MachineInstr *VPST = Insts.front();
+        auto DivergentNext = ++MachineBasicBlock::iterator(Divergent);
+        bool DivergentNextIsPredicated =
----------------
dmgreen wrote:
> We are sure here that Divergent will not be nullptr? So ++ on the iterator is valid?
There is guaranteed to be a divergent since the block is not uniformly predicated.


================
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())) {
----------------
dmgreen wrote:
> 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.
This is the one at the start of the loop body block. I can't quite wrap my head around the changes your suggesting. Perhaps they should come as a separate patch later on?


================
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:
----------------
dmgreen wrote:
> Can you remove hidden and  the "local_unnamed_addr #0"
Ah yes, I missed that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90461