[PATCH] D121899: [LoopVectorize] Optimise away the icmp when tail-folding for some low trip counts

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8812
+                        {CanonicalIVIncrement, &Plan.getVectorTripCount(),
+                         Plan.getOrCreateTripCount()},
+                        DL);
----------------
sdesmalen wrote:
> There's nothing wrong with it, but perhaps it's a bit unfortunate this has to be passed into the VPInstruction as an operand. It seems more like a bit of useful knowledge of the loop that the VPInstruction could use to optimise the branch, rather than an actual operand for the conceptual BranchOnCount intrinsic. If the trip-count of the loop is constant, maybe we can store that information as a piece of state somewhere.
> 
> @fhahn what would be the desired place to add such information about the loop? My understanding was that the uses of the InnerLoopVectorizer in VPlan were being phased out, is that right?
Yeah I'm not terribly happy about adding this as an operand, but I wasn't sure how else to pass this. I could possibly add a constant trip count to the VPTransformState, which is available when generating the instruction?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:750-760
+    uint64_t TCVal = 0;
+    if (auto *TC2 = dyn_cast<ConstantInt>(TC))
+      TCVal = TC2->getZExtValue();
+
+    Value *Cond;
+    // When we know there will only be one vector iteration there is no need to
+    // create the comparison, since we already know the answer.
----------------
sdesmalen wrote:
> nit: How about
> 
>   Value *ConstCmp = nullptr;
>   if (auto *C = dyn_cast<ConstantInt>(TC))
>     if (C->getZExtValue() <= State.UF * State.VF.getKnownMinValue())
>       ConstCmp = Builder.getInt1(true);
>   Value *Cond = ConstCmp ? ConstCmp : Builder.CreateICmpEQ(IV, VTC);
It would have to be something like this I think:

```Value *ConstCmp = nullptr;
if (auto *C = dyn_cast<ConstantInt>(TC)) {
  uint64_t TCVal = C->getZExtValue();
  if (TCVal && TCVal  <= State.UF * State.VF.getKnownMinValue())
    ConstCmp = Builder.getInt1(true);
}
Value *Cond = ConstCmp ? ConstCmp : Builder.CreateICmpEQ(IV, VTC);```

because I'm pretty sure I found a test where the trip count was actually defined as a zero constant. I'm happy to change it to the code above, but not sure it looks much better to be honest. :)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:978
   // modeled explicitly in VPlan.
-  cast<Instruction>(LastBranch->getCondition())->moveBefore(LastBranch);
+  if (isa<Instruction>(LastBranch->getCondition()))
+    cast<Instruction>(LastBranch->getCondition())->moveBefore(LastBranch);
----------------
sdesmalen wrote:
> I noticed some of this code here has been removed and I'm not sure if this change is then still required. In any case this patch needs a rebase.
Yeah I noticed that too. Will fix in a new patch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121899