Page MenuHomePhabricator

[Analysis] add optional index parameter to isSplatValue()
ClosedPublic

Authored by spatel on Jan 28 2020, 6:40 AM.

Details

Summary

We want to allow splat value transforms to improve PR44588 and related bugs:
https://bugs.llvm.org/show_bug.cgi?id=44588
...but to do that, we need to know if values are splatted from the same, specific index (lane) rather than splatted from an arbitrary index.

We can improve the undef handling with 1-liner follow-ups because the Constant API optionally allow undefs now.

This change will cause a conflict with the planned update of shufflevector's mask in D72467, but that's a small fix-up depending on if it lands first or second.

Diff Detail

Event Timeline

spatel created this revision.Jan 28 2020, 6:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2020, 6:40 AM

I don't fully understand how this API is going to be used. The current API makes sense to me (are all elements in the vector the same?) but the new one is unclear, especially for the case where no shufflevector is involved. If I have a splat constant, which index does the value come from?

Do you already have a followup patch that uses this API that you could share?

I don't fully understand how this API is going to be used. The current API makes sense to me (are all elements in the vector the same?) but the new one is unclear, especially for the case where no shufflevector is involved. If I have a splat constant, which index does the value come from?

I agree it's not clear, so I'm certainly open to recommendations for improvement. For now, the index parameter is ignored on a splat constant. The TODO comment suggests a follow-up:

// FIXME: We can allow undefs, but if Index was specified, we may want to
//        check that the constant is defined at that index.

Do you already have a followup patch that uses this API that you could share?

Posted as D73703 - that has a test example where we are splatting from 2 different indexes, so we can't reassociate the binops.

efriedma added inline comments.Jan 30 2020, 1:06 PM
llvm/include/llvm/Analysis/VectorUtils.h
310

This description is really misleading.

The simple definition of a "splat", if we didn't have poison or undef, would simply be that we have to prove that each element is equal. (The index wouldn't be relevant.)

If you add poison, it becomes something like "Each element is either poisoned or equal to every other non-poisoned element. In addition, if an index is specified, either every element of the vector is poisoned, or the element at that index is not poisoned".

I haven't tried to concisely extend that to include undef, but the description should still look roughly like that.

spatel updated this revision to Diff 241742.Jan 31 2020, 8:43 AM
spatel marked an inline comment as done.

Patch updated:
Changed documentation comment for isSplatValue().

efriedma added inline comments.Jan 31 2020, 10:09 AM
llvm/lib/Analysis/VectorUtils.cpp
362

Please use getMaskValue here.

lebedev.ri accepted this revision.Jan 31 2020, 10:47 AM

This seems good to me.

This revision is now accepted and ready to land.Jan 31 2020, 10:47 AM
spatel updated this revision to Diff 241776.Jan 31 2020, 11:15 AM
spatel marked an inline comment as done.

Patch updated:
Use ShuffleVectorInst API to make the code more flexible (should not conflict with D72467); this also allows removing the assert from the previous rev (similar assert is included within getMaskValue()).

This revision was automatically updated to reflect the committed changes.