Page MenuHomePhabricator

[VPlan] Add VPlan based sinkInstructions utility.
AcceptedPublic

Authored by fhahn on May 14 2018, 3:59 AM.

Details

Summary

This patch introduces a sinkInstructions utility that sinks instructions
in a VPlan based on an Instruction -> Instruction mapping, as returned
by LoopVectorizationLegality::getSinkAfter. It also adds a
VPlanHCFGTransforms.cpp, for functions that do not rely on stuff
defined in LoopVectorize.cpp, to move to a more modular structure.

Once we migrated getSinkAfter to operate on a VPlan, we should change
this to not rely on the underlying values.

Diff Detail

Event Timeline

fhahn created this revision.May 14 2018, 3:59 AM

Hi Florian. Some comments, regardless my previous comments in D46827:

  1. Location: Same as in D46827, we may need to find a better place. Maybe a VPlanHCFGTransforms class for now?
  2. Just a thought for the future: I think sinkAfter could be one of those things that we may want to rethink/re-implement a bit in the VPlan native path. If it's only used for interleave groups, we may want to include the sinkAfter analysis and transformation steps as part of the future interleave groups VPlan-to-VPlan transformation.

Thanks,
Diego

lib/Transforms/Vectorize/LoopVectorizationPlanner.cpp
20 ↗(On Diff #146576)

I'm using independent DEBUG_TYPE in D44338 for VPlan verifier and HCFGConstruction files. I guess we should be consistent. We could use 'loop-vectorize' for all of them or also use an independent one for this.

lib/Transforms/Vectorize/LoopVectorizationPlanner.h
258 ↗(On Diff #146576)

Is \p still used for documenting parameters?

260 ↗(On Diff #146576)

format: beyond 80 chars?

fhahn updated this revision to Diff 146858.May 15 2018, 9:37 AM
fhahn marked 2 inline comments as done.
fhahn edited the summary of this revision. (Show Details)

Thanks Diego, LoopVectorizationPlanner is not the right place, initially I was not sure where to put it. I updated the patch to move sinkInstructions to VPlanHCFGTransforms. I've made this a class with static member functions, as sinkInstructions gets access to getUnderlyingVal via a friend class.

I agree that in the long term, we should migrate to a VPlan native handling of the interleave group handling. I suppose that would be something that could be done independently. Maybe it would be worth collecting a set of tasks/cleanups that aren't blocked on other changes.

fhahn added inline comments.May 15 2018, 9:38 AM
lib/Transforms/Vectorize/LoopVectorizationPlanner.cpp
20 ↗(On Diff #146576)

I think using 'loop-vectorize' everywhere is that we can use -debug-only=loop-vectorize to get the complete picture.

unittests/Transforms/Vectorize/VPlanTest.cpp
55

This version leaks the created instructions. I will clean that up in the next update (or before committing).

dcaballe accepted this revision.May 15 2018, 8:09 PM

Much better with this VPlanHCFGTransforms! Thanks a lot!
Just some minor comments. LGTM. Again, let's wait for D46827 to see if this code is still necessary.

Thanks!

lib/Transforms/Vectorize/VPlanHCFGTransforms.cpp
26

assert(isa) + cast?

35–36

same here? assert(isa) + cast?

lib/Transforms/Vectorize/VPlanValue.h
41

Hopefully this won't promote the use of the underlying IR for other VPlan-to-VPlan transformations when it's not necessary.

This revision is now accepted and ready to land.May 15 2018, 8:09 PM
rja accepted this revision.May 16 2018, 12:37 AM
rja added a subscriber: rja.

LGTM

fhahn added a comment.Jun 14 2018, 5:23 AM

After the recent changes to D46827, this patch is not really necessary for now, but it may be useful in the future.