This is an archive of the discontinued LLVM Phabricator instance.

[DagCombiner] Allow shuffles to merge through bitcasts
ClosedPublic

Authored by RKSimon on Feb 27 2015, 7:23 AM.

Details

Summary

Currently shuffles may only be combined if they are of the same type, despite the fact that bitcasts are often introduced in between shuffle nodes (e.g. x86 shuffle type widening).

This patch allows a single input shuffle to peek through bitcasts and if the input is another shuffle will merge them, shuffling using the smallest sized type, and re-applying the bitcasts at the inputs and output instead.

Dropped old ShuffleToZext test - this patch removes the use of zext and vector-zext.ll covers these anyhow.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 20853.Feb 27 2015, 7:23 AM
RKSimon retitled this revision from to [DagCombiner] Allow shuffles to merge through bitcasts.
RKSimon updated this object.
RKSimon edited the test plan for this revision. (Show Details)
RKSimon added reviewers: chandlerc, andreadb, qcolombet.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: Unknown Object (MLST).
RKSimon updated this revision to Diff 21102.Mar 3 2015, 6:58 AM

Ping. Rebased and added early-out.

qcolombet added inline comments.Mar 3 2015, 2:09 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11921

I don’t think this assert stands in a pre legalization world.
This is perfectly legal AFAIK to do something like this:
bitcast <4 x i48> %a to <6 x i32>

test/CodeGen/X86/vector-shuffle-128-v16.ll
1352

Do you actually need the zero initializer here?
“undef" should do the trick and would make more obvious why you patch apply.

test/CodeGen/X86/vector-shuffle-128-v2.ll
827

Ditto.

test/CodeGen/X86/vector-shuffle-128-v4.ll
1589

Ditto.

RKSimon updated this revision to Diff 21198.Mar 4 2015, 7:22 AM

Thanks Quentin. The assert for re-scalable scalar types has been changed into part of the type legality test - I've also used that opportunity to remove the code duplication between scaling the inner or outer shuffles. I've also updated the tests to use undef where possible.

andreadb accepted this revision.Mar 5 2015, 4:30 AM
andreadb edited edge metadata.

Hi Simon,

Overall the patch looks good to me. I suggest that you move this new logic into a separate function. In my opinion, It would make the code in visitVECTOR_SHUFFLE a bit more readable. I also made a couple of minor comments (see below).

Thanks!
-Andrea

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11883

In this context, we know that N0 is used by N, so writing
'N->isOnlyUserOf(N0.getNode())' is equivalent to writing 'N0.hasOneUse()'. If there is one use, then it can only be N.

11899–11907

A very minor nit:
You can probably explicitly initialize NewMask like this:
SmallVector<int, 8> NewMask(Scale, -1);

This will allow you to simplify the loop using operator[]. So, NewMask.push_back(Scale * M + i) would become NewMask[i] = Scale * M + i.
Also, if you do that, then you would not need to always 'push_back(-1)' if M is less than 0.

This revision is now accepted and ready to land.Mar 5 2015, 4:30 AM
This revision was automatically updated to reflect the committed changes.

Thanks Andrea, I was able to include some of your minors in this patch. I'll be sending a follow up patch soon (probably next week now) that will remove all the code duplication we have for shuffle mask commutation.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp