Page MenuHomePhabricator

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

Authored by fhahn on Apr 12 2020, 8:02 AM.



After introducing VPWidenSelectRecipe, the duplicated logic can be

Diff Detail

Event Timeline

fhahn created this revision.Apr 12 2020, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2020, 8:02 AM
Ayal added a comment.Apr 12 2020, 4:15 PM

Yes, this follows D77972 naturally :-)


Can add !isa<PHINode>(I) to this assertion list after sinking tryToWidenSelect() as suggested below.


How about doing here (tryToCreateRecipe should have a better name btw):

  if (!shouldWiden(I, Range))
    return false;

  if ((Recipe = tryToWidenSelect(Instr)) ||
      (isa<GetElementPtrInst>(Instr) &&
       (Recipe = new VPWidenGEPRecipe(GEP, OrigLoop)))) ||
     (Recipe = tryToWiden(Instr, *Plan))) {
    setRecipe(Instr, Recipe);
    return true;

  return false;
fhahn updated this revision to Diff 256946.Apr 13 2020, 2:00 AM

Consolidate widening checks for GEP, Select and general widening as suggested in tryToCreateRecipe.

Ayal accepted this revision.Apr 13 2020, 2:53 AM

Thanks for cleaning this up! Final couple of nits.


Retain the !isa<Call, Load, Store> alongside !isa<PHI>?
To avoid using this method for such opcodes, as their widening decision is more complex.


More natural to reverse the conditions and rename willWiden to willScalarize, returning !getDecisionAndClampRange(willScalarize, Range)?


Branches should also find their way out of IsVectorizableOpcode and into an assert; no recipes are created for them.

This revision is now accepted and ready to land.Apr 13 2020, 2:53 AM
fhahn updated this revision to Diff 257225.Apr 14 2020, 1:18 AM
fhahn marked an inline comment as done.

Change willWiden to willScalarize.

(This patch depends on D77973)

fhahn updated this revision to Diff 257230.Apr 14 2020, 1:33 AM

Avoid calling shouldWiden for calls, loads and stores by exiting earlier. They should be widened by the specialized recipes or scalarized.

fhahn marked 2 inline comments as done.Apr 14 2020, 1:39 AM
fhahn added inline comments.

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?


Will submit as a follow-up commit once this one lands.

Ayal added inline comments.Apr 14 2020, 7:42 AM

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) && ...);


fhahn marked an inline comment as done.Apr 14 2020, 8:27 AM
fhahn added inline comments.

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.)

Ayal added inline comments.Apr 14 2020, 12:13 PM

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);
fhahn marked an inline comment as done.Apr 14 2020, 12:56 PM
fhahn added inline comments.

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 :)

This revision was automatically updated to reflect the committed changes.