Page MenuHomePhabricator

[VPlan] VPlan version of InterleavedAccessInfo.
ClosedPublic

Authored by fhahn on Jul 18 2018, 8:21 AM.

Details

Summary

This patch turns InterleaveGroup into a template with the instruction type
being a template parameter. It also adds a VPInterleavedAccessInfo class, which
only contains a mapping from VPInstructions to their respective InterleaveGroup.
As we do not have access to scalar evolution in VPlan, we can re-use
convert InterleavedAccessInfo to VPInterleavedAccess info.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Jul 18 2018, 8:21 AM

I like the templatization approach taken here. Thank you!. Changes look fine to me, but will wait for a few others to take a look.

rkruppe added inline comments.Jul 19 2018, 5:39 AM
lib/Transforms/Vectorize/VPlan.cpp
599 ↗(On Diff #156077)

It doesn't seem like this memory is freed anywhere.

Reading D49491 it seems to me that the InterleaveGroups allocated here are only needed while the VPInterleavedAccessInfo object is alive, so it could free them in its destructor (or even better, store unique_ptrs that take care of that automatically).

fhahn updated this revision to Diff 156310.Jul 19 2018, 10:24 AM

Delete allocated InterleaveGroups in destructor.

lib/Transforms/Vectorize/VPlan.cpp
599 ↗(On Diff #156077)

Yep thanks! They should be freed in VPInterleavedAccessInfo's destructor.

Just a few comments.

Thanks!
Diego

lib/Transforms/Vectorize/VPlan.cpp
584 ↗(On Diff #156310)

This algorithm assumes that there is only a region (top region) in the whole H-CFG but this is going to change pretty soon. Could you please change the implementation to recursively go inside other potential nested regions?

591 ↗(On Diff #156310)

auto * for casts and dyn_cast (585, 591, 592)?

lib/Transforms/Vectorize/VPlan.h
1337 ↗(On Diff #156310)

Just curious. How difficult would it be to templatize the IAI methods needed here?

fhahn added inline comments.Aug 9 2018, 9:35 AM
lib/Transforms/Vectorize/VPlan.cpp
584 ↗(On Diff #156310)

Will do. I just have to think a bit more how everything will fit together with outerloops, as the regular InterleavedAccessInfo only works on a single loop.

lib/Transforms/Vectorize/VPlan.h
1337 ↗(On Diff #156310)

I am not entirely sure, but most of the regular IAI is related to building the interleave groups, which is not required here because we re-using the regular one.

fhahn updated this revision to Diff 173825.Nov 13 2018, 2:34 AM
fhahn marked an inline comment as done.

Rebased, updated the use auto * for cast<>.

fhahn updated this revision to Diff 173829.Nov 13 2018, 3:37 AM
fhahn marked an inline comment as done.

Updated to traverse regions.

So, IIUC, the way you only get the interleave info on instructions and map to VPlan is because we don't yet have scalar evolution in VPlan, so we need to do that in Instruction and then map to VPInstruction.

In the future, once we can (perhaps using the same template technique) have SCEV on VPlan, we won't need the Old2New map anymore, and then the analysis will be independent of the underlying type.

Di I get that right?

fhahn added a comment.Nov 13 2018, 6:35 AM

So, IIUC, the way you only get the interleave info on instructions and map to VPlan is because we don't yet have scalar evolution in VPlan, so we need to do that in Instruction and then map to VPInstruction.

In the future, once we can (perhaps using the same template technique) have SCEV on VPlan, we won't need the Old2New map anymore, and then the analysis will be independent of the underlying type.

Di I get that right?

Exactly, using the underlying plain IR values is used for the initial implementation, until we have something like SCEV for VPlan and can do the whole analysis on top of VPlan.

rengolin accepted this revision.Nov 13 2018, 6:46 AM

Perfect, LGTM. Thanks!

This revision is now accepted and ready to land.Nov 13 2018, 6:46 AM
This revision was automatically updated to reflect the committed changes.