Page MenuHomePhabricator

[LV] Changing the interface of ValueMap
ClosedPublic

Authored by Ayal on Jun 21 2017, 1:00 PM.

Details

Summary

Following the review of D32871:

An alternative interface which may be simpler and clearer is to only support setting and getting a single Value per part, or per part-and-lane. Setting a value can assert that no value has already been set; this assert can be overridden when needed using a specialized "resetting" method. This keeps the implementation of *MapStorage internal to ValueMap, albeit potentially being less efficient as it avoids batching (if not inlined). If so, it may be better done in a separate patch. Sounds reasonable?

This is that separate patch.

Diff Detail

Repository
rL LLVM

Event Timeline

Ayal created this revision.Jun 21 2017, 1:00 PM
mssimpso edited edge metadata.Jun 23 2017, 12:42 PM

Hi Ayal,

Sorry for the long turn-around on this. This looks mostly fine to me. I think it would make sense to commit this prior to the VPlan patch since it's conceptually a separate change. Committing first would also ensure that we keep the interface clean, throughout.

Michael, do you have any issues with moving to this new interface for the ValueMap?

lib/Transforms/Vectorize/LoopVectorize.cpp
685–687 ↗(On Diff #103443)

Don't we still need to declare this functions, and the others, as friends?

699 ↗(On Diff #103443)

Entry[Part].resize(VF) null initializes the values, right?

2449–2450 ↗(On Diff #103443)

It might be more efficient in the common case if we pulled this out of the loop. Though, I don't really have a strong preference.

if (isa<TruncInst>(EntryVal))
  for (unsigned Part = 0; Part < UF; ++Part)
    addMetadata(getOrCreateVectorValue(EntryVal, Part);

Hopefully, I've understood the interface correctly...

2563–2564 ↗(On Diff #103443)

Same as above.

2706 ↗(On Diff #103443)

Do we need to check/assert that the map contains a non-null entry for each Lane? Is it possible that some of the entries would be null?

Maybe uniform-after-vectorization values have null entries for VF > 1? If this is still the case, it might be useful to re-think that choice. It might be simpler to just broadcast the VF == 1 value to the other lanes instead of leaving them null.

4038 ↗(On Diff #103443)

Why do we use VectorLoopValueMap.getVectorValue() here instead of getOrCreateVectorValue()? Is it because we know the Phi must exist in the map already? This makes sense to me.

I think in the original code, we had to use VectorLoopValueMap.getVector() to get a non-const reference.

4219 ↗(On Diff #103443)

Can't we just use VectorLoopValueMap.getVectorValue(LoopExitInst, Part), etc.?

mkuper edited edge metadata.Jun 23 2017, 4:32 PM

Michael, do you have any issues with moving to this new interface for the ValueMap?

Makes sense to me, modulo a couple of nits.

lib/Transforms/Vectorize/LoopVectorize.cpp
711 ↗(On Diff #103443)

Since unset is only ever used before set, I think a nicer interface would be to have "reset" that combines an unset with a set.
But if you prefer this, I'm ok with it.

3005 ↗(On Diff #103443)

This comment doesn't quite parse.

Ayal added inline comments.Jun 24 2017, 3:39 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
685–687 ↗(On Diff #103443)

Exposing this interface that manipulates individual Values only, keeps their storage internal to ValueMap, w/o providing any external befriended method access to it. This follows the thought of eventually moving this interface out of ILV and into a standalone storage class.

699 ↗(On Diff #103443)

Ah, sure, if we remember to do Entry[Part].resize(VF, nullptr) ... Good catch, Thanks!

711 ↗(On Diff #103443)

Having "reset" was our original thought too, will return to it. Having "unset" may also undermine hasAnyVectorValue() btw.

2449–2450 ↗(On Diff #103443)

We could indeed unswitch this invariant "if" manually; but its also good to reuse LastInduction rather than retrieve it. The trip count is probably small for this to have noticeable impact. It may also be conceptually better to keep addMetadata() right next to setVectorValue().

2563–2564 ↗(On Diff #103443)

ditto

2706 ↗(On Diff #103443)

As originally documented, this check is essentially asking if the value has been scalarized, to any lane/part. For that we can (continue to) check if the map contains the value, w/o looking at any specific lane/parts. Perhaps it would be better instead to ask directly if the decision is to scalarize this value (going forward, by consulting the associated VPlan recipe), instead of asking ValueMap for this information.

The uniform-after-vectorization values are indeed entered per lane 0 only, keeping other lanes null. Another way to re-think this choice is to allocate only UF entries for such values, and separate the type (scalar/vector) of the values being stored from how many values are being stored (per part, or per part-and-lane). Can add a TODO comment.

3005 ↗(On Diff #103443)

Right; will fix the comment as we move it.

4038 ↗(On Diff #103443)

Yes, we use VectorLoopValueMap.getVectorValue(Phi, 0) here because we know the Phi must already exist in the map; the original code asserts so, and should have also asserted that PhiParts[0] is not null, as it is passed to SetInsertPoint().

Agreed, the original code uses VectorLoopValueMap.getVector(Phi) mainly to get a non-const reference for (re)setting PhiPart[Part] = Shuffle further below.

4219 ↗(On Diff #103443)

We could just use VectorLoopValueMap.getVectorValue(LoopExitInst, Part) and hold a single Value *RdxPart instead of holding the temporary VectorParts RdxParts(UF), if the for-part loop creating the SExt/Zext's is fused with the for-part loop creating the Trunc's; this requires moving the insertion point back and forth...

We should indeed do this in the for-part loop below which creates a BinOp or MinMaxOp. Will do so in a revised version to be uploaded.

Ayal updated this revision to Diff 103855.Jun 24 2017, 4:23 PM

Updated version addresses review comments.

mkuper accepted this revision.Jun 26 2017, 8:42 AM

LGTM, but please wait for Matt.

lib/Transforms/Vectorize/LoopVectorize.cpp
699 ↗(On Diff #103443)

I'm pretty sure it null-initializes implicitly, but explicit is always good. :-)

This revision is now accepted and ready to land.Jun 26 2017, 8:42 AM
mssimpso accepted this revision.Jun 26 2017, 10:25 AM

LGTM as well. Thanks!

This revision was automatically updated to reflect the committed changes.