This is an archive of the discontinued LLVM Phabricator instance.

[X86] Try to combine vector anyext + and into a vector zext
ClosedPublic

Authored by mkuper on Feb 16 2015, 3:36 AM.

Details

Summary

Vector zext tends to get legalized into a vector anyext (represented as a vector shuffle with an undef vector + a bitcast) that gets ANDed with a mask that zeroes the undef elements.
Combine this into an explicit shuffle with a zero vector instead. This allows shuffle lowering to match it as a zext, instead of matching it as an anyext and emitting an explicit and like it does now.

This doesn't cover all the cases, as you can see in the test, but it's a start.

Diff Detail

Repository
rL LLVM

Event Timeline

mkuper updated this revision to Diff 20013.Feb 16 2015, 3:36 AM
mkuper retitled this revision from to [X86] Try to combine vector anyext + and into a vector zext.
mkuper updated this object.
mkuper edited the test plan for this revision. (Show Details)
mkuper added reviewers: chandlerc, andreadb, RKSimon.
mkuper added a subscriber: Unknown Object (MLST).
andreadb accepted this revision.Feb 16 2015, 5:06 AM
andreadb edited edge metadata.

Hi Michael,

Overall the change looks good to me. There are a couple of things that could be improved (see comments below).

In the description, you mentioned that this rule would help simplifying a (type-)legalized dag node sequence obtained from a vector zext. It probably makes sense to guard this new rule against DCI.isBeforeLegalize(). If the level is 'BeforeLegalizeTypes' then we probably don't want/need to enable this new rule.

Minor question: what about moving this logic into a separate function? I don't have a strong opinion about it, so feel free to keep the code in 'PerformAndCombine' if you think it makes more sense.

lib/Target/X86/X86ISelLowering.cpp
24668–24669 ↗(On Diff #20013)

I think you can remove these two checks.
Your algorithm already checks (between line 24682 and 24686) that N1 is a build_vector or a bitcast of a build_vector.

24691 ↗(On Diff #20013)

You don't need a dyn_cast here. Node Splat can only be an ISD::BUILD_VECTOR at this point.
I think you can safely use a 'cast' here.

This revision is now accepted and ready to land.Feb 16 2015, 5:06 AM

Thanks a lot for the review, Andrea!

You're right, it deserves its own function.

Regarding legalization - I think it should be guarded by BeforeLegalizeOps(), I've accidentally put it before the guard.
I could make it BeforeLegalize(), but that's probably a bit redundant, since, at least for the cases I've looked at the vector_shuffle in question is created as part of vector ops legalization.

lib/Target/X86/X86ISelLowering.cpp
24668–24669 ↗(On Diff #20013)

Right, either the check here or the second check (in 24685) is redundant.
I'd rather remove the check in 24685, to be honest.

24691 ↗(On Diff #20013)

Right.

mkuper added inline comments.Feb 16 2015, 7:28 AM
lib/Target/X86/X86ISelLowering.cpp
24668–24669 ↗(On Diff #20013)

Never mind, ignore this, you're right.

This revision was automatically updated to reflect the committed changes.