[PATCH] D113223: [VPlan] Add VPCanonicalIVRecipe, partly retire createInductionVariable.

[Florian Hahn via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2533
+    Value *ScalarIV = CanonicalIV;
+    if (!isCanonicalID(ID) || CanonicalIV->getType() != IV->getType()) {
       ScalarIV = IV->getType()->isIntegerTy()
----------------
Ayal wrote:
> Slightly more consistent to check `ScalarIV->getType() != IV->getType()`, given the cast from one to the other below.
> 
> May be good to assign `auto NeededType = IV->getType()` ?
I added the new variable, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8924
+
+  auto *CanonicalIV = new VPCanonicalIVPHIRecipe(StartV, DL);
+  VPRegionBlock *TopRegion = Plan.getVectorLoopRegion();
----------------
Ayal wrote:
> "CanonicalIV" >> "CanonicalIVPhi", to complement "CanonicalIVIncrement".
Adjusted!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9674
+      IV, getInductionDescriptor(), getStartValue()->getLiveInIRValue(),
+      getTruncInst(), getVPValue(0), State, CanonicalIV);
 }
----------------
Ayal wrote:
> getVPValue(0) >> getVPSingleValue()?
There's no need to use a `getVP*Value` accessor, `this` can passed directly. Updated!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:972
 
+  if (VectorTripCount.getNumUsers()) {
+    O << "\nLive-in ";
----------------
Ayal wrote:
> VectorTripCount is always used by the compare of branch-on-count, and by it only. So its VPValue is always generated but will have a user only once the compare (possibly w/ its increment and branch) is also represented as a recipe/VPInstruction. For now can assert it has no users; code introduced otherwise is dead.
Good point, I replaced the printing with assert, will move the printing to the patch that uses it explicitly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113223