[PATCH] D139074: Vectorization Of Conditional Statements Using BOSCC

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

It looks like the patch doesn't apply cleanly any longer. Could you rebase it?



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:904
+class BOSCCBlockPlanner {
+private:
+  BasicBlock *BB;
----------------
no need for `private:` 


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8048
+  // This can be improved later.
+  return (BB->sizeWithoutDebug() > BOSCCInstructionInBlockThreshold);
+}
----------------
Did you find any cases where the transform isn't profitable? Shouldn't this take into account at least the potential savings if the block isn't executed together with the amount of execution units/resource saturation? 

Also, should this take into account profile info and avoid the transform if the block is executed frequently?




================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9129
+          VPJoinBlock->appendRecipe(LiveOutRecipe);
+          // RecipeBuilder.resetRecipe(Instr, LiveOutRecipe);
+          Builder.setInsertPoint(VPBB);
----------------
dead code?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9150
+        VPJoinBlock->appendRecipe(LiveOutRecipe);
+        // RecipeBuilder.resetRecipe(Instr, LiveOutRecipe);
+        Builder.setInsertPoint(VPBB);
----------------
dead code?


================
Comment at: llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h:126
 
+  void resetRecipe(Instruction *I, VPRecipeBase *R) {
+    Ingredient2Recipe[I] = R;
----------------
Is this needed? If possible that should avoided.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1720
+private:
+  VPBasicBlock *VPGuardBlock;
+  VPBasicBlock *VPVecContinueBlock;
----------------
The recipe shouldn't directly reference VPBasicBlocks; this may cause issues if the VPBasicBlocks are renmoved or replaced for example. Is it possible to use the predecessors in the CFG in the VPlan?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2116
   RecipeListTy Recipes;
+  bool IsBOSCCBlock;
 
----------------
Could you explain why this is needed (as well as `markBOSCCBlock` and `isBOSCCBlock`)?  It would be good to avoid having to put those details into `VPBasicBlock`.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2635
+  /// Indicates whether VPlan contains BOSCC Blocks
+  bool HasBOSCCBlocks = false;
+
----------------
Is this needed? If a VPlan transform removes a BSOCC block then this will become out-of-date.


================
Comment at: llvm/test/Transforms/LoopVectorize/boscc0.ll:12
+
+; Function Attrs: argmemonly nofree norecurse nosync nounwind uwtable
+define dso_local void @foo(ptr noalias nocapture noundef writeonly %A, ptr nocapture noundef readonly %B, ptr nocapture noundef readonly %C, ptr noalias nocapture noundef readnone %D, ptr nocapture noundef readnone %E, ptr nocapture noundef readnone %F, ptr nocapture noundef readonly %X, i32 noundef %len) local_unnamed_addr #0 {
----------------
Please try to clean up the input IR for the test. It appears like there many unused arguments, function attributes and metadata.


================
Comment at: llvm/test/Transforms/LoopVectorize/boscc0.ll:90
+entry:
+  %cmp12.not = icmp eq i32 %len, 0
+  br i1 %cmp12.not, label %for.cond.cleanup, label %for.body.preheader
----------------
check should not be needed and can be simplified.


================
Comment at: llvm/test/Transforms/LoopVectorize/boscc0.ll:94
+for.body.preheader:                               ; preds = %entry
+  %wide.trip.count = zext i32 %len to i64
+  br label %for.body
----------------
just pass `%len` as `i64` so there's no need to `zext`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139074