[PATCH] D111846: [LV] Drop integer poison-generating flags from instructions that need predication

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the update! I left a few small remaining comments. Basically LGTM, but it would still be good to hear @nlopes thoughts on the recent responses with respect to widened instructions.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1214
+      auto *Parent = dyn_cast_or_null<VPRegionBlock>(CurRec->getParent());
+      Instruction *Instr = CurRec->getUnderlyingInstr();
+      auto *RepRec = dyn_cast<VPReplicateRecipe>(CurRec);
----------------
I think it would be helpful to add a comment spelling out what we are looking for here.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1217
+      if ((!RepRec || !RepRec->isPredicated()) && Instr &&
+          (!Parent || !Parent->isReplicator()) && State.VF.isVector() &&
+          cast<Operator>(Instr)->hasPoisonGeneratingFlags())
----------------
I'm not entirely sure why we need to check the parent region/replicator? It would be great if you could include the reasoning in the comment above, or remove if it is not needed.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1232
+  auto Iter = depth_first(
+      VPBlockRecursiveTraversalWrapper<VPBlockBase *>(State.Plan->getEntry()));
+  for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>(Iter)) {
----------------
nit: it might be more explicit to pass the plan as argument instead of accessing it from State.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111846