This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Add InductionDescriptor to VPWidenIntOrFpInduction. (NFC)
ClosedPublic

Authored by fhahn on Dec 5 2021, 5:02 AM.

Details

Summary

This allows easier access to the induction descriptor from VPlan,
without needing to go through Legal. VPReductionPHIRecipe already
contains a RecurrenceDescriptor in a similar fashion.

Diff Detail

Event Timeline

fhahn created this revision.Dec 5 2021, 5:02 AM
fhahn requested review of this revision.Dec 5 2021, 5:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 5 2021, 5:02 AM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added inline comments.Dec 8 2021, 3:46 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8591

This change from lookup() to find() is done in order to obtain a reference to an existing InductionDescriptor to be recorded in VPWidenIntOrFpInduction recipe, rather than a copy thereof. Right?

Can alternatively do

if (!Legal->isInductionPhi(Phi))
  return nullptr;
InductionDescriptor &II = Legal->getInductionVars().find(Phi)->second;
8623

This is a similar setting of II as suggested above, but where checking isInductionPhi() is redundant.

llvm/lib/Transforms/Vectorize/VPlan.h
65

Lex order

1068

IndDesc >> Descriptor?
Or, to better match VPReductionPHIRecipe, getDescriptor() >> getInductionDescriptor()?

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
49

A similar case, albeit w/o direct access to Legal.
Should Legal support all above cases by providing something like:

Optional<InductionDescriptor *> getInductionDescriptor(PHINode *Phi) {
  if (!isInductionPhi(Phi))
    return None;
  return &getInductionVars().find(Phi)->second;
}

or even

Optional<InductionDescriptor *> getIntOrFpInductionDescriptor(PHINode *Phi) {
  if (!isInductionPhi(Phi))
    return None;
  auto &ID = getInductionVars().find(Phi)->second;
  if (ID.getKind() == InductionDescriptor::IK_IntInduction ||
      ID.getKind() == InductionDescriptor::IK_FpInduction)
    return &ID;
  return None;
}

?

fhahn updated this revision to Diff 393089.Dec 9 2021, 2:54 AM
fhahn marked 2 inline comments as done.

Adjust naming, add and use getIntOrFpInductionDescriptor helper as suggested.

fhahn marked 3 inline comments as done.Dec 9 2021, 2:56 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8591

I went ahead and added a getIntOrFpInductionDescriptor helper as suggested below.

llvm/lib/Transforms/Vectorize/VPlan.h
65

Moved to correct position.

1068

updated to getInductionDescriptor.

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
49

I went ahead and added a getIntOrFpInductionDescriptor helper. It is used in LV and here. VPInstructionsToVPRecipes has been updated to take a function_ref to get use it. That should reduce duplication to a minimum

Ayal accepted this revision.Dec 9 2021, 3:21 AM

Thanks! Ship it, just a couple of typos to fix.

llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
313

[f]or

llvm/lib/Transforms/Vectorize/VPlan.h
1067

to >> the

llvm/lib/Transforms/Vectorize/VPlanTransforms.h
26

Lex order.

This revision is now accepted and ready to land.Dec 9 2021, 3:21 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked 2 inline comments as done.
fhahn marked 3 inline comments as done.Dec 10 2021, 1:56 AM
fhahn added inline comments.
llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
313

Thanks, should be fixed in the committed version.

llvm/lib/Transforms/Vectorize/VPlan.h
1067

Thanks, should be fixed in the committed version.

llvm/lib/Transforms/Vectorize/VPlanTransforms.h
26

Thanks, should be fixed in the committed version.

fhahn marked 3 inline comments as done.Dec 10 2021, 3:47 AM