This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Clean up tryToCreate(Widen)Recipe. (NFC)
ClosedPublic

Authored by fhahn on Apr 16 2020, 2:52 AM.

Details

Summary

This patch includes some clean-ups to tryToCreateRecipe, suggested in
D77973.

It includes:

  • Renaming tryToCreateRecipe to tryToCreateWidenRecipe.
  • Move VPBB insertion logic to caller of tryToCreateWidenRecipe.
  • Hoists instruction checks to tryToCreateWidenRecipe, making it clearer which instructions are handled by which recipe, simplifying the checks by using early exits.
  • Split up handling of induction PHIs and truncates using inductions.

Diff Detail

Event Timeline

fhahn created this revision.Apr 16 2020, 2:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2020, 2:52 AM
Ayal accepted this revision.Apr 19 2020, 4:03 AM

Thanks for following up on this! Looks good to me with a couple of nits.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
6867

No need to cast I into TruncInst.

6892

Orthogonal thought: perhaps if the constructor of VPBlendRecipe would receive two iterator_ranges, one for the values and one for the masks, and fuse them itself, tryToBlend() (or createBlendRecipe()) could be simplified using map_ranges.

7085

It would be clearer to further refactor the handling of Phi's into

if (Phi->getParent() != OrigLoop->getHeader())
  return tryToBlend(Phi, Plan); // Rename to createBlendRecipe()?
if (Recipe = tryToOptimizeInductionPHI(Phi))
  return Recipe;
return new VPWidenPHIRecipe(Phi); // Rename to VPWidenHeaderPHIRecipe()?
llvm/lib/Transforms/Vectorize/VPRecipeBuilder.h
108

Above comment needs updating.

121

nit: [is] can

This revision is now accepted and ready to land.Apr 19 2020, 4:03 AM
fhahn marked 4 inline comments as done.Apr 20 2020, 2:01 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7085

Thanks, I'll include the changes in the commit. Renaming could be done as follow-up, together with the suggestion re BlendRecipe above.

This revision was automatically updated to reflect the committed changes.