This is an archive of the discontinued LLVM Phabricator instance.

[SLP]Need to shrink the load vector after reordering.
AbandonedPublic

Authored by ABataev on Jan 19 2021, 7:37 AM.

Details

Summary

After merging the shuffles, we cannot rely on the previous shuffle
anymore and need to shrink the final shuffle, if it is required.

Reported in D92668.

The previous patch in D93967 was reverted because the original code had a bug. This patch fixes it.

Diff Detail

Event Timeline

ABataev created this revision.Jan 19 2021, 7:37 AM
ABataev requested review of this revision.Jan 19 2021, 7:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2021, 7:37 AM

Is the summary correct? The patch that this is fixing isn't reverted...

It's not clear what this is fixing and why it's fixing it.

Thanks!

-eric

Is the summary correct? The patch that this is fixing isn't reverted...

It's not clear what this is fixing and why it's fixing it.

Thanks!

-eric

Here is the reverting commit https://reviews.llvm.org/rGbcbdeafa9cb3469a7bf367c17f32d913076a4d55

Before D92668, we could reuse the argument of the shuffle as the instruction and in most cases, we did this. And it masked the issue that exists in the code for the case when there is no shuffle after vectorization of the node. After D92668 we could not reuse the argument of the shuffle anymore and should resize the vectorized instructions always if required. But originally we did it incorrectly. If some of the scalars are duplicated in the vectorization tree entry, we do not vectorize them but instead generate a mask for the reuses. But if there are several users of the same entry, they may have different vectorization factors. This is especially important for PHI nodes. In this case, we need to adapt the resulting instruction for the user vectorization factor and have to reshuffle it again to take only unique elements of the vector. Before this patch, the compiler incorrectly returned reduced vector instruction with the same elements, not with the unique ones.

block:
%phi = phi <2 x > { .., %entry} {%shuffle, %block}
%2 = shuffle <2 x > %phi, %poison, <4 x > <0, 0, 1, 1>
... (use %2)
%shuffle = shuffle <2 x> %2, poison, <2 x> {0, 2} <----------------- Currently the compiler emits {0, 1} here but insteasd it should be {0, 2}, otherwise %2 instruction becomes incorrect after the first iteration
br %block

I think this would be more clear as a combined patch rather than 3 separate and some reverted patches. Would it make more sense to revert the chain and put them all back together in a single review?

I think this would be more clear as a combined patch rather than 3 separate and some reverted patches. Would it make more sense to revert the chain and put them all back together in a single review?

There is no chain, actually. And the patch, that caused the problem, is not directly related.
D92668 just improves the cost model by squashing multiple shuffles. This one fixes the issue in the code that existed even before D92668 and D92668 just revealed it.

Hard to tell because there aren't any comments around any of the code that changed in either patch. Can you fix that in a separate change and actually document what everything is doing? :)

For this patch can you add a comment that explains what we're doing here as well. It's unclear.

Hard to tell because there aren't any comments around any of the code that changed in either patch. Can you fix that in a separate change and actually document what everything is doing? :)

This is a separate patch with the fix only. :) There are no more patches required to fix the issue, just this one. Or do you mean something else?

For this patch can you add a comment that explains what we're doing here as well. It's unclear.

Sure.

Hard to tell because there aren't any comments around any of the code that changed in either patch. Can you fix that in a separate change and actually document what everything is doing? :)

This is a separate patch with the fix only. :) There are no more patches required to fix the issue, just this one. Or do you mean something else?

The previous patch.. and yeah, I know it's not your fault some of this could be better documented and thanks for fixing it up :)

For this patch can you add a comment that explains what we're doing here as well. It's unclear.

Sure.

Thanks!

-eric

ABataev abandoned this revision.Jan 19 2021, 12:11 PM

Hmm?

I just reverted the original patch for shuffles and going to merge this patch and the patch for shuffles. Going to post it soon.