[PATCH] D135017: [LV] Move exit cond simplification to separate transform.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 3 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7623
+  if (!IsEpilogueVectorization)
+    VPlanTransforms::optimizeForConcreteVFAndUF(BestVPlan, BestVF, BestUF, PSE);
+
----------------
Ayal wrote:
> nit: can drop "Concrete".
> 
> Restrict the VF range of BestPlan to include only BestVF?
> 
> When printing BestPlan would be good to now indicate that it applies to BestUF only.
> nit: can drop "Concrete".

Done!

> Restrict the VF range of BestPlan to include only BestVF?

Done!

> When printing BestPlan would be good to now indicate that it applies to BestUF only.

Should I add a field for that?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:464
+      Plan.getCanonicalIV()->getStartValue()->getLiveInIRValue()->getType();
+  const SCEV *ExitCount = createTripCountSCEV(IdxTy, PSE);
+  auto *C = dyn_cast<SCEVConstant>(ExitCount);
----------------
Ayal wrote:
> TripCountV indeed hasn't been created yet, but perhaps there's a simpler way to check this condition, e.g., by checking if PSE.getBackedgeCount() is a constant greater than VF*UF-1, instead of incrementing it by 1 of type IdxTy; or trying to call SE.getSmallConstantTripCount(L); or recording it in VPlan, possibly in CanonicalIV recipe.
That's what I started out with, but the trip count (BTC + 1) allows additional simplifications in D133017 as BTC's often involve a `-1`, which means it may be negative limiting SCEV simplifications.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:474
+      new VPInstruction(VPInstruction::BranchOnCond,
+                        {Plan.getOrAddExternalDef(ConstantInt::getTrue(Ctx))});
+  Term->eraseFromParent();
----------------
Ayal wrote:
> Worth an unconditional branch VPInstruction instead of the effort to generate a redundant True? Can be fixed independent of this patch, which is working a bit harder to generate it.
It should probably be sufficient to just set a single successor. We will have to also replace & remove the header phis.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:66
   static void removeRedundantExpandSCEVRecipes(VPlan &Plan);
+
+  static void optimizeForConcreteVFAndUF(VPlan &Plan, ElementCount VF,
----------------
Ayal wrote:
> Missing documentation.
added, thanks!


================
Comment at: llvm/test/Transforms/LoopVectorize/lcssa-crashes.ll:16
 ; CHECK-NEXT:    [[INDEX_NEXT]] = add nuw i64 [[INDEX]], 4
-; CHECK-NEXT:    br i1 true, label [[MIDDLE_BLOCK:%.*]], label [[VECTOR_BODY]], !llvm.loop [[LOOP0:![0-9]+]]
+; CHECK-NEXT:    [[TMP1:%.*]] = icmp eq i64 [[INDEX_NEXT]], 0
+; CHECK-NEXT:    br i1 [[TMP1]], label [[MIDDLE_BLOCK:%.*]], label [[VECTOR_BODY]], !llvm.loop [[LOOP0:![0-9]+]]
----------------
Ayal wrote:
> Is this a regression from previously folding the conditional branch?
The test case is spurious, the vector body will never be executed, as the exit condition compares to undef.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135017