This is an archive of the discontinued LLVM Phabricator instance.

[LV] Record GEP widening decisions in recipe (NFCI)
ClosedPublic

Authored by gilr on Oct 16 2019, 12:59 PM.

Details

Summary

InnerLoopVectorizer's code called during VPlan execution still relies on
original IR's def-use relations to decide which vector code to generate,
limiting VPlan transformations ability to modify def-use relations and still
have ILV generate the vector code.
This commit moves GEP operand queries controlling how GEPs are widened to a
dedicated recipe and extracts GEP widening code to its own ILV method taking
those recorded decisions as arguments. This reduces ingredient def-use usage by
ILV as a step towards full VPlan-based def-use relations.

Diff Detail

Event Timeline

gilr created this revision.Oct 16 2019, 12:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2019, 12:59 PM
Ayal added a comment.Oct 31 2019, 7:57 AM

Having special treatment for GEPs here does follow the unique way in which GEPs tolerate both scalar/invariant operands and/or vector operands.

Separate the VPWidenMemoryInstructionRecipe changes from those of VPWidenGEPRecipe, into two patches?

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

A GEP having only loop-invariant operands should be treated like any other uniform instruction, i.e., by a VPReplicateRecipe rather than a VPWidenGEPRecipe. This TODO is related to D68831 BTW.

6930

Would it be better and/or more in line with the current scheme, if this handling of GEPs by a designated Recipe took place outside the generic tryToWiden() which employs the default VPWidenRecipe?

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

Keep in lex order.

745

Update comment from phi to gep.

750

Holding a LoopInvariantIndices set would be worse than IsIndexLoopInvariant bit vector, right?

gilr marked 4 inline comments as done.Nov 3 2019, 7:55 AM
gilr added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.h
750

I think so.

gilr updated this revision to Diff 227614.Nov 3 2019, 8:16 AM

Address comments.

gilr updated this revision to Diff 230879.Nov 25 2019, 5:32 AM
gilr retitled this revision from [LV] Reduce ingredient DU usage by ILV (NFC) to [LV] Record GEP widening decisions in recipe (NFCI).
gilr edited the summary of this revision. (Show Details)
  • Limit this commit to handling GEP widening only, per Ayal's comment.
  • Bug fix: The all-operands-are-invariant check was not checking the pointer's invariance.
Ayal added a comment.Nov 26 2019, 8:13 AM

This looks good to me, thanks, plus a couple of comments.

Worth noting that GEPs will now stop the extensibility of WidenRecipe's compaction optimization. But this compaction should eventually be removed to represent def-use relations.

Let's wait a few days if anyone else would like to review.

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

Can use GEP->indices() instead.

6898

Checking if I isa PHINode here is completely redundant, given that all phi's were handled before tryToWiden(). But this cleanup calls for a separate patch.

6939–6940

This setRecipe handles GEPs, and should continue to do so.

7036

Missing setRecipe(...) of the new recipe here.

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

Can also print the in/variance information, although printing the GEP seems sufficient.

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

Can use GEP->indices() instead.

gilr marked 5 inline comments as done.Dec 1 2019, 1:09 AM
gilr updated this revision to Diff 231612.Dec 1 2019, 1:34 AM

Addressed comments.

Ayal accepted this revision.Dec 5 2019, 1:39 AM
This revision is now accepted and ready to land.Dec 5 2019, 1:39 AM
fhahn accepted this revision.Dec 5 2019, 1:55 AM

LGTM, thanks for working on this!

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

nit: you could also use enumerate(GEP->indices()) and get rid of the extra counter I, which might be a bit more explicit

gilr marked an inline comment as done.Dec 6 2019, 2:18 AM
gilr updated this revision to Diff 232517.Dec 6 2019, 2:26 AM

Addressed final comment (thanks Florian!)

This revision was automatically updated to reflect the committed changes.