This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Allow build_vector-to-vector_shuffle combine to combine builds from two overly-wide vectors.
ClosedPublic

Authored by mkuper on Sep 12 2016, 7:42 PM.

Details

Summary

This allows us to create a vector_shuffle out of a build_vector, when we have extract_elements from two inputs, at least one of which is wider than the output.
(E.g. a <8 x i16> being constructed out of elements from a <16 x i16> and a <8 x i16>).

This is the first patch in a series - the idea is to get all of the "oddshuffles" tests to use shuffles instead of insert/extract sequences. Most (all?) of the remaining cases require building a vector out of more than 2 inputs.

Diff Detail

Repository
rL LLVM

Event Timeline

mkuper updated this revision to Diff 71100.Sep 12 2016, 7:42 PM
mkuper retitled this revision from to [DAG] Allow build_vector-to-vector_shuffle combine to combine builds from two overly-wide vectors..
mkuper updated this object.
mkuper added reviewers: andreadb, RKSimon, spatel.
mkuper added a subscriber: llvm-commits.
ABataev added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13054–13055 ↗(On Diff #71100)
if (ShuffleNumElems > NumElems)
  Mask.append(ShuffleNumElems-NumElems, -1)

?

mkuper added inline comments.Sep 13 2016, 1:15 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
13054–13055 ↗(On Diff #71100)

Yeah, I keep writing these loops on auto-pilot. :-)
This should either be an append, or Mask should just be initialized to -1. Thanks!

RKSimon edited edge metadata.Sep 13 2016, 3:21 AM

Thanks for looking at this! I've made a few minor observations.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12996 ↗(On Diff #71100)

Its not necessarily twice the size here is it? I'm thinking xmm from zmm.

13054–13055 ↗(On Diff #71100)

It'd be tidier if you just did SmallVector<int, 8> Mask(ShuffleNumElems, -1) and replaced the push_back() calls with Mask[i]

13079 ↗(On Diff #71100)

Use ZeroIdx?

andreadb accepted this revision.Sep 13 2016, 3:22 AM
andreadb edited edge metadata.

Hi Michael,

The patch looks good to me . I only have one comment (see below).

-Andrea

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12962–12973 ↗(On Diff #71100)

My understanding is that we check if VT is a legal type for the target at the very beginning of this method.

However, (correct me if I am wrong) we don't check if InVT1 and InVT2 are legal types. So, we potentially always enable this canonicalization even on illegal vector types.

I wonder if we should add a early exit for the case where the DAG is not "type legalized" and InVT1 and InVT2 are not legal types for the target. The legalizer would adjust/canonicalize those vector types. We would still end up running your code if eventually types still don't match VT.

Not sure if this makes sense.

This revision is now accepted and ready to land.Sep 13 2016, 3:22 AM

Thanks Alexey, Simon, Andrea!

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12962–12973 ↗(On Diff #71100)

I'm not entirely sure. If the inputs are longer than the output, you're probably right.
The case that worries me, though, is something like a build_vector of <4 x i32> from 2 * <2 x i32>. I think we may want to hit this pre-legalization.
I'll add (yet another :-) ) TODO, and we can think about this post-commit. I'm going to keep working on this code anyway.

12996 ↗(On Diff #71100)

You're right, that should be another TODO. (This isn't a change made in this patch, we've always supported it only in the "twice the size" case).

13054–13055 ↗(On Diff #71100)

Yes, that's what I meant by "should just be initialized to -1".
I think we've already had this conversation in a previous patch, as I said, this is just auto-pilot. Thanks again!

13079 ↗(On Diff #71100)

Right, just need to define it in the right scope.

This revision was automatically updated to reflect the committed changes.