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

[Diego Caballero via Phabricator via llvm-commits]

dcaballe added a comment.

Thanks, Florian! I addressed the feedback.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1217
+      if ((!RepRec || !RepRec->isPredicated()) && Instr &&
+          (!Parent || !Parent->isReplicator()) && State.VF.isVector() &&
+          cast<Operator>(Instr)->hasPoisonGeneratingFlags())
----------------
fhahn wrote:
> 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.
Good catch! Actually most of these conditions are redundant or irrelevant in the new algorithm since we already know this recipe is contributing to the address computation of a VPWidenMemoryRecipe or VPInterleaveRecipe. That simplifies a lot the condition. Thanks!


================
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)) {
----------------
fhahn wrote:
> nit: it might be more explicit to pass the plan as argument instead of accessing it from State.
We are also using other fields from `State` here, like `VF` and `MayGeneratePoisonRecipes` and this is the only used of `Plan`. I'm not sure if it's worth it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111846