[PATCH] D77973: [VPlan] Move widening check for non-memory/non-calls to function (NFC).

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6940
+bool VPRecipeBuilder::shouldWiden(Instruction *I, VFRange &Range) const {
+  assert(!isa<PHINode>(I) && "PHIs should have been handled earlier");
+  // Instruction should be widened, unless it is scalar after vectorization,
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > fhahn wrote:
> > > > Ayal wrote:
> > > > > Retain the !isa<Call, Load, Store> alongside !isa<PHI>?
> > > > > To avoid using this method for such opcodes, as their widening decision is more complex.
> > > > I've brought it back, but with the new code structure, it also required adding an early exit for calls, loads & stores, but I think that makes sense. What do you think?
> > > Ahh, right. Perhaps better to early-exit here:
> > > 
> > > 
> > > ```
> > >   // Calls and memory instructions that were not widened earlier should be scalarized.
> > >   if (isa<CallInst>(Instr) || isa<LoadInst>(Instr) || isa<StoreInst>(Instr))
> > >     return false;
> > >   assert(!isa<PHINode>(I) && ...);
> > > ```
> > Is there an advantage to have the early exit here? To me it seems to fit slightly better at the call site of shouldWiden, because there it is clearer what is happening (i.e. the code to handle calls, stores, loads is just above.)
> Your original version is fine, as approved. Advantage of last suggestion was merely to avoid an obvious assert immediately following an early-exiting.
> 
> Thoughts of further cleanup:
> 
>   - have tryToCreate[Widen]Recipe return a VPRecipeBase* instead of bool,
>   - have its caller take care of { setRecipe(Instr, Recipe); VPBB->appendRecipe(Recipe); }; either using another method of VPRecipeBuilder or exposing its setRecipe() to LVP.
>   - split tryToOptimizeInduction in two: first part that handles PHI and second that handles Trunc
>   - refactor out various isa's from tryTo's and fold the early-exits closer: 
> 
> 
> ```
> VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe(Instruction *Instr, VFRange &Range, VPlanPtr &Plan) {
>   if (auto *CI = dyn_cast<CallInst>(Instr))
>     return tryToWidenCall(CI, Range, *Plan);
>   if (isa<LoadInst>(Instr) || isa<StoreInst>(Instr))
>     return tryToWidenMemory(Instr, Range, Plan);
>   if (auto *PHI = dyn_cast<PHINode>(Instr)) {
>     if (auto *Recipe = tryToOptimizeInduction(PHI, Range))
>       return Recipe;
>     if (auto *Recipe = tryToBlend(PHI, Plan))
>       return Recipe;
>     return new VPWidenPHIRecipe(PHI);
>   }
>   if (auto *Trunc = dyn_cast<TruncInst>(Instr))
>     if (auto *Recipe = tryToOptimizeIVTruncate(Trunc, Range))
>       return Recipe;
>   
>   if (!shouldWiden(Instr, Range))
>     return nullptr;
> 
>   if (auto *GEP = dyn_cast<GetElementPtrInst>(Instr))
>     return new VPWidenGEPRecipe(GEP, OrigLoop);
>   if (auto *Select = dyn_cast<SelectInst<(Instr))
>     return tryToWidenSelect(Instr);
>   return tryToWiden(Instr, *Plan);
> }
> ```
SGTM, I just wanted to keep this relatively self-contained. It would be great if you could also take another look at D77972, which is required for this patch to land :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77973