Page MenuHomePhabricator

[CodeGen] Update transformations to use poison for shufflevector/insertelem's initial vector elem
ClosedPublic

Authored by aqjune on Mon, Jan 4, 7:39 PM.

Details

Summary

This patch is a part of D93817 and makes transformations in CodeGen use poison for shufflevector/insertelem's initial vector element.

The change in CodeGenPrepare.cpp is fine because the mask of shufflevector should be always zero.
It doesn't touch the second element (which is poison).

The change in InterleavedAccessPass.cpp is also fine becauses the mask is of the form <a, a+m, a+2m, .., a+km> where a+km is smaller than
the size of the first vector operand.
This is guaranteed by the caller of replaceBinOpShuffles, which is lowerInterleavedLoad.
It calls isDeInterleaveMask and isDeInterleaveMaskOfFactor to check the mask is the desirable form.
isDeInterleaveMask has the check that a+km is smaller than the vector size.
To check my understanding, I added an assertion & added a test to show that this optimization doesn't fire in such case.

Diff Detail

Event Timeline

aqjune created this revision.Mon, Jan 4, 7:39 PM
aqjune requested review of this revision.Mon, Jan 4, 7:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jan 4, 7:39 PM
spatel accepted this revision.Tue, Jan 5, 11:32 AM

LGTM - other reviewers are probably more familiar with InterleavedAccess than me, so might want to wait for a 2nd opinion.
I made another assert suggestion for that code. Similar to the assert that you have proposed in this patch, we could add the asserts before this patch just to make the shuffle assumptions clearer (assuming those are correct assertions).

llvm/lib/CodeGen/InterleavedAccessPass.cpp
363–365

Could we also assert here (and/or in the loop above this one) that:

VecTy->getNumElements() < Shuffle->getShuffleMask().size()

?

Ie, the shuffle is always reducing the number of elements from the input vector operands?

This revision is now accepted and ready to land.Tue, Jan 5, 11:32 AM
nikic added inline comments.Tue, Jan 5, 11:38 AM
llvm/lib/CodeGen/CodeGenPrepare.cpp
6722

Wondering if you can replace this insertelement + shufflevector by a CreateVectorSplat call.

llvm/lib/CodeGen/InterleavedAccessPass.cpp
405

Why is this checking only the back() element? Better check all? assert(llvm::all_of(Mask, [&](int Idx) { return Idx < ... });

aqjune added inline comments.Tue, Jan 5, 8:11 PM
llvm/lib/CodeGen/InterleavedAccessPass.cpp
363–365

Yes, I'll add one.

405

It was simply because of its cost. Actually I was wondering whether it is acceptable to have such formula in the assertion.

aqjune updated this revision to Diff 315656.Sun, Jan 10, 1:04 AM

Address comments

aqjune marked 3 inline comments as done.Sun, Jan 10, 1:05 AM
This revision was landed with ongoing or failed builds.Sun, Jan 10, 1:06 AM
This revision was automatically updated to reflect the committed changes.