This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Add VPDef class.
ClosedPublic

Authored by fhahn on Nov 1 2020, 10:40 AM.

Details

Summary

This patch introduces a new VPDef class, which can be used to
manage VPValues defined by recipes/VPInstructions.

The idea here is to mirror VPUser for values defined by a recipe. A
VPDef can produce either zero (e.g. a store recipe), one (most recipes)
or multiple (VPInterleaveRecipe) result VPValues.

To traverse the def-use chain from a VPDef to its users, one has to
traverse the users of all values defined by a VPDef.

VPValues now contain a pointer to their corresponding VPDef, if one
exists. To traverse the def-use chain upwards from a VPValue, we first
need to check if the VPValue is defined by a VPDef. If it does not have
a VPDef, this means we have a VPValue that is not directly defined
iniside the plan and we are done.

If we have a VPDef, it is defined inside the region by a recipe, which
is a VPUser, and the upwards def-use chain traversal continues by
traversing all its operands.

Note that we need to add an additional field to to VPVAlue to link them
to their defs. The space increase is going to be offset by being able to
remove the SubclassID field in future patches.

Diff Detail

Event Timeline

fhahn created this revision.Nov 1 2020, 10:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2020, 10:40 AM
fhahn requested review of this revision.Nov 1 2020, 10:40 AM
fhahn updated this revision to Diff 303513.Nov 6 2020, 11:50 AM

add some additional assertions, comments and make VPDef not inherit from VPUser, to allow more flexibility for now.

Ayal added a comment.Nov 13 2020, 12:34 AM

Looks good to me, adding minor comments, thanks for pushing this forward!

In the summary:

If we have a VPDef, we have to traverse all operands of the VPDef.

If we have a VPDef, it is defined inside the region by a recipe, which is a VPUser, and the upwards def-use chain traversal continues by traversing all its operands.

llvm/lib/Transforms/Vectorize/VPlanValue.h
38

Try to keep in lex order?

51

ditto...

101–102

detach from Def?

235

augment[s]

243

Worth having 'protected' rather than 'private', to allow recipes access?

May also need the ability to remove/replace a defined value.

272

'defined values' >> 'values defined'

273

Agree with tidy: defined_values() >> definedValues() or getDefinedValues().

llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
524

VPMultiValuedDef or VPDoubleValuedDef? It's a single Def, with multiple Values.

525

Unused; suffice to hold the VPValues by VPDef alone.

529

Add delete's, for completeness? Adding them inside ~VPMultiDef() will kill the VPValues before ~VPDef() checks they have no users and checks/resets their Def's... have ~VPValue() take itself out of its VPDef, as commented above?
Or delete the VPValues after deleting the VPMultiDef...

fhahn updated this revision to Diff 305175.Nov 13 2020, 8:43 AM
fhahn marked 7 inline comments as done.

Thanks for taking a look! Addressed the comments & also added a bit more comments and updated VectorizationPlan.rst

Ayal added inline comments.Nov 14 2020, 2:28 PM
llvm/lib/Transforms/Vectorize/VPlanValue.h
268

Better avoid deleting all VPValues here - at-least those who are VPInstructions/single-valued-recipes?

fhahn added a comment.Nov 14 2020, 2:54 PM

I realised I added a couple of response I didn't submit. Here they are

llvm/lib/Transforms/Vectorize/VPlanValue.h
101–102

Done, now the destructor removes the VPValue from its def.

243

Worth having 'protected' rather than 'private', to allow recipes access?

I'd prefer to change to protected once we need to?

May also need the ability to remove/replace a defined value.

I've added removeDefinedValue

268

I think conceptually it would make sense that deleting a VPDef also deletes all values defined by it?

The single value def case should work out I think, because the VPVAlue destructor should be called before the VPDef one, and the VPValue destructor removes the value from DefinedValues.

If you think it's too subtle though we can delegate this to the concrete subclasses that define multiple values.

llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
524

The naming is still from earlier versions. Changed to VPDoubleValueDef.

525

Yep, left over from an earlier version. Removed now.

529

I updated the VPDef destructor to delete all defined values. So no delete's should be needed here.

Ayal added inline comments.Nov 15 2020, 12:45 AM
llvm/lib/Transforms/Vectorize/VPlanValue.h
250

/// Add >> /// Remove

268

Ahh, ok, D90565 changes the multiple inheritance order of VPInstruction, to first inherit from VPRecipeBase and last from VPValue, so indeed the latter's destructor (which now removes itself from its VPDef) should be called before the former. Worth a comment.

fhahn updated this revision to Diff 305349.Nov 15 2020, 3:54 AM

Fix comment, expand VPDef doc-comment to state VPDefs own their defined VPValues and are responsible for deleting them. Also mention inheritance order when inheriting from both VPValue and VPDef.

llvm/lib/Transforms/Vectorize/VPlanValue.h
268

Exactly. I added more details to the VPDef doc-comment. Is there another place that should have additional comments?

Ayal accepted this revision.Nov 15 2020, 9:08 AM

This looks good to me, thanks!

llvm/lib/Transforms/Vectorize/VPlan.cpp
69

nit: this assert that removeDefinedValue(V) sets V->Def to null seems a bit redundant; its comment describes what is done if (Def) ...

llvm/lib/Transforms/Vectorize/VPlanValue.h
268

Very good, thanks.
Probably worth noting also in D90565 when setting the (new) multiple inheritance order.

This revision is now accepted and ready to land.Nov 15 2020, 9:08 AM
fhahn updated this revision to Diff 305372.Nov 15 2020, 9:57 AM

This looks good to me, thanks!

Thank you very much Ayal! I'll wait a bit with committing this, in case there are additional comments.

I also removed an over-cautious assert.

llvm/lib/Transforms/Vectorize/VPlan.cpp
69

Yes that's probably over-cautious. I removed it.

fhahn edited the summary of this revision. (Show Details)Nov 17 2020, 1:41 AM
This revision was landed with ongoing or failed builds.Nov 17 2020, 8:18 AM
This revision was automatically updated to reflect the committed changes.