[PATCH] D139074: Vectorization Of Conditional Statements Using BOSCC

[Ashutosh Nema via Phabricator via llvm-commits]

Ashutosh marked 6 inline comments as done.
Ashutosh added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8048
+  // This can be improved later.
+  return (BB->sizeWithoutDebug() > BOSCCInstructionInBlockThreshold);
+}
----------------
fhahn wrote:
> 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?
> 
> 
Having an additional check is a tradeoff, in general we have experienced covering expensive or large block is beneficial compared to small sized block.
Definitely with the help of profile info we can make more precise judgements.
I'll check the possibility to include that.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1720
+private:
+  VPBasicBlock *VPGuardBlock;
+  VPBasicBlock *VPVecContinueBlock;
----------------
fhahn wrote:
> 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?
I understood your point, but for this recipe its important to know the guard block and continue block.

Using this info it will add the required incomings to the live out PHI.

Possibly we can maintain the mapping for this information in VPlan and while executing VPBOSCCLiveOutRecipe it can infer the details from it ?

If you have any alternatives please suggest.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2116
   RecipeListTy Recipes;
+  bool IsBOSCCBlock;
 
----------------
fhahn wrote:
> 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`.
This is required to understand the BOSCC blocks during mergeBlocksIntoPredecessors.
It merges the BOSCC blocks, which troubles the live out generations.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D139074