[PATCH] [x86] Begin a significant overhaul of how vector lowering is done in the x86 backend.
Chandler Carruth
chandlerc at gmail.com
Wed Jun 25 17:58:26 PDT 2014
OK, I think I've responded to all the comments reasonably at this point. Will upload an updated patch momentarily. Please let me know if this LG to submit at this point! =D
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6769
@@ +6768,3 @@
+ for (int i = 0, Size = Mask.size(); i < Size; ++i)
+ if (Mask[i] != -1 && Mask[i] != i)
+ return false;
----------------
Jim Grosbach wrote:
> Chandler Carruth wrote:
> > Jim Grosbach wrote:
> > > You comment on this below, but IMO it's worth calling out explicitly here that UNDEF masks are required to be implemented as preserving the input lane and thus are considered a NOP.
> > >
> > > This and isSingleInputShuffleMask() should probably be static methods of ShuffleVectorSDNode instead. That would be similar to how isSplatMask() is implemented (side note, that should probably switch to use an ArrayRef instead of an EVT). Then add simple non-static wrappers isNoopShuffle() and isSingleInputShuff() to the class which call down to the static versions using the instance's mask.
> > Regarding the undef mask contract below, it doesn't impact this at all. An undef lane, by its nature, is a NOP?
> >
> >
> > I really dislike static methods for these at the moment. These have nothing to do with an SDNode and everything to do with the specific ways in which masks are used in this lowering code. I'm hesitant to over-generalize here.
> >
> > Also, static methods are horrible to actually use.
> >
> > If there is useful common logic to refactor into common locations, I'd rather do it if and when it is actually needed to implement common functionality. As it stands, it wouldn't save anything?
> I'm more concerned that it's inconsistent with what we already have (isSplatMask()). I don't care much which idiom we use (static method vs. static function), but do think we should be consistent.
>
> I disagree that they have nothing to do with the SDNode. They're for asking questions about a mask either of or for an SDNode. Now, strictly speaking, the mask is a distinct thing, it's true, and we could have a dedicated class or namespace for that, I suppose.
>
> Static methods are no harder to use than the static functions you already have. It's just a scope.
>
> On the other hand, do other targets actually want answers to the questions these helpers ask? If not, maybe you're right that they're more closely related to the X86 lowering code than the generic stuff. So I suggest we get an answer pragmatically. Can you have a look at the other targets that do shuffles and see if they have something similar? If so, combine the use cases with something generic and if not, leave it as you've written it currently. Sound reasonable?
Chatted about this on IRC some and I've add lots of comments which will even explain the specific peculiarities of these routines.
The essence is that these seem pretyt special-purpose today. They work on raw masks, and those masks sometimes have fairly specific meaning to this cod ethat might not generalize well. I Actually tried to move the single input test and it is just wrong in general; it only works here with this code path.
Hopefully with the documentation this helps clarify what's going on.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6782
@@ +6781,3 @@
+// NB: We rely heavily on "undef" masks preserving the input lane.
+static SDValue getV4ShuffleImmForMask(ArrayRef<int> Mask, SelectionDAG &DAG) {
+ assert(Mask.size() == 4 && "Only 4-lane shuffle masks");
----------------
Filipe Cabecinhas wrote:
> This is similar to a (DAG) shuffle mask, but not quite (it only goes up (non-inclusive) to 4, not 8, which you would have on shuffles for two v4* vectors). Could you add some word (Shufp?) to the name, to make it easier to spot the difference?
Do you have a concrete suggestion here? I don't have any ideas other than V4. We use this for SHUFP, PSHUF, PSHUFH, and PSHUFL instructions. =/
The way I think about it is that this is just the way that x86 always represents a 4-lane shuffle, regardless of where the 4 lanes come from.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6803
@@ +6802,3 @@
+ ArrayRef<int> Mask = SVOp->getMask();
+ assert(Mask.size() == 2 && "Unexpected mask size for v2 shuffle!");
+
----------------
Filipe Cabecinhas wrote:
> Chandler Carruth wrote:
> > Jim Grosbach wrote:
> > > Likewise, it may be useful to expose a direct method on ShuffleVectorSDNode for getting the lane count.
> > >
> > > int getNumElements() const { return getValueType(0).getVectorNumElements(); }
> > I don't know that this makes the code any better. By directly asserting properties of the Mask, to me, it makes the following code that assumes the properties on the mask much more clear.
> Shouldn't we test, at a higher level, that the shuffle's type is v2f64? I don't think we can get a malformed SDValue that's a v2f64 with more (or fewer) than 2 elements in the mask. The same for the other functions.
>
Yea, testing this stuff is a great idea. I've added lots of asserts to this effect. Also fixed a bunch of places where we were playing fast and loose with types to instead go back through the full vector shuffle code to ensure type correctness throughout.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6808
@@ +6807,3 @@
+ // sharing the input and destination.
+ unsigned SHUFPDMask = (Mask[0] & 1u) | ((Mask[1] & 1u) << 1u);
+ return DAG.getNode(X86ISD::SHUFP, SDLoc(Op), MVT::v2f64, V1, V1,
----------------
Filipe Cabecinhas wrote:
> Since the possible values are UNDEF (-1), 0, and 1, wouldn't simply Mask[x] == 1u be easier to understand (This would invert how we handle UNDEFs in this case)
Sure.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6813
@@ +6812,3 @@
+ assert(Mask[0] >= 0 && Mask[0] < 2);
+ assert(Mask[1] >= 2);
+
----------------
Jim Grosbach wrote:
> Chandler Carruth wrote:
> > Jim Grosbach wrote:
> > > Comment strings on the asserts.
> > When there is a generally useful bit of information that is being asserted, I try to do so. However, these asserts are just documenting the mathematical invariants established by the above condition. There isn't any useful comment to give here IMO.
> I'd still prefer there be something. As a matter of consistent style, if nothing else. Just something that which tells me what other properties I should be looking at to see what happened. For example, distinguishing between a mask that's invalid on its face for the vector type vs. one that's valid but shouldn't have gotten this far because the code above was supposed to handle it.
I've added strings to every assert. I've no idea of many of these really add value, but hey. =D
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6816
@@ +6815,3 @@
+ unsigned SHUFPDMask = (Mask[0] & 1u) | (((Mask[1] - 2) & 1u) << 1u);
+ return DAG.getNode(X86ISD::SHUFP, SDLoc(Op), MVT::v2f64, V1, V2,
+ DAG.getConstant(SHUFPDMask, MVT::i8));
----------------
Filipe Cabecinhas wrote:
> I might be missing something. How does this handle an <i32 2, i32 0> mask? Wouldn't it generate
>
> { V1[0], V2[0] }
>
> instead of
>
> { V2[0], V1[0] }
>
> ?
How? There is a test case that covers this. ;]
I think the key is that we canonicalize the mask first, so we know that by the time we gete here there is exactly one entry from each input, and when we have equal numbers of inptus, we canonicalize s.t. that first input provides more low-half inputs than the second input.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6836
@@ +6835,3 @@
+ std::max(Mask[1], 0) * 2, std::max(Mask[1], 0) * 2 + 1};
+ return DAG.getNode(X86ISD::PSHUFD, SDLoc(Op), MVT::v4i32, V1,
+ getV4ShuffleImmForMask(WidenedMask, DAG));
----------------
Filipe Cabecinhas wrote:
> What about bitcasting the PSHUFD output back to v2i64?
Done. I think the DAG stuff was fixing this for me. This code is definitely covered by tests. =/
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6859
@@ +6858,3 @@
+ SDValue LowV = V1, HighV = V2;
+ int NewMask[4] = {Mask[0], Mask[1], Mask[2], Mask[3]};
+
----------------
Jim Grosbach wrote:
> Chandler Carruth wrote:
> > Jim Grosbach wrote:
> > > space after "{" and before "}"
> > See the coding guidelines and clang-format -- this is how it formats braces specifically to interoperate cleanly with braced init lists in C++11.
> clang-format agrees with me. That's why I made the comment, actually.
To record here what took place on IRC -- Jim's build of clang-format was stale for truly mysterious reasons... no idea why...
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6874
@@ +6873,3 @@
+ ++V2Index;
+ int V2AdjIndex = V2Index ^ 1;
+
----------------
Jim Grosbach wrote:
> Chandler Carruth wrote:
> > Jim Grosbach wrote:
> > > This is a neat trick for getting the other lane of the high half, but it isn't immediately obvious that's what it's doing. Either a ?: to make the selection explicit or a comment explaining the bitwise trick would be good.
> > I have a distaste for comments which merely explain what the standard says the C++ in question does.
> >
> > I tried to help this by naming the variable after the "adjacent" index. Is there a better variable name that would give the context? If not I can do something else...
> It's not just explaining what the standard says, though. It explains why. I spend a couple minutes looking at that code wondering what you were up to before I realized and I had to work through the bit patterns in my head to do it. Maybe I'm just being dense and it should have been obvious, but it wasn't. I really think a comment explaining the intent (not the language semantics) is very useful here.
>
> The abbreviation certainly didn't help. I thought it meant "Adjusted" not "Adjacent".
Comments added.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6886
@@ +6885,3 @@
+ // blended with a V1 element. Regardless of how many other V1 elements we
+ // have, this will always require a two-step blend.
+ int V1Index = V2AdjIndex;
----------------
Jim Grosbach wrote:
> Chandler Carruth wrote:
> > Jim Grosbach wrote:
> > > Big ask, but I'd love some ASCII art type examples of the transforms, in particular for those like this one that need more than one instruction. Not the complete set of possibilities; just a single example that's illustrative showing arrows for which lanes get moved where by which insn.
> > I'm hesitant to do this. Every time I drew an ascii art picture (and I drew several) or something on a white board, it actually messed me up because I started to silently assume that the one example I had was actually representative of the other ways to hit the same scenario.
> >
> > Unless this is breaking your ability to reason about the code, I'm going to hold off investing a ton of time in crafting these...
> I'm drawing lots of stuff on paper to be able to reason about the code. Some comments to help guide that thought process would help.
>
> From another perspective, comments breaking down what the code is intended to cover help review to make sure it matches what it actually does cover.
>
> I understand that a mathematically sound exposition of all the cases and how they transpose into one another is likely not practical, but I still think a few example would be useful.
>
> Doesn't have to be ASCII art. I just like pictures. Simple examples, even expressed in terms of __builtin_shufflevector() .c code perhaps, would go a long way.
Ok.
However, this comment was just bad. Bordering on terrible. I've rewritten it to be less confusing. =]
I'm not sure after that exactly where diagrams or examples would most help? Could you point at the specific places that need more illustration in light of the added comments from the last couple of iterations?
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6917
@@ +6916,3 @@
+ (Mask[0] >= 4 ? Mask[0] : Mask[1]) - 4,
+ (Mask[2] >= 4 ? Mask[2] : Mask[3]) - 4};
+ V1 = DAG.getNode(X86ISD::SHUFP, DL, MVT::v4f32, V1, V2,
----------------
Jim Grosbach wrote:
> Chandler Carruth wrote:
> > Jim Grosbach wrote:
> > > Spacing for "{" "}".
> > Same comment, this is the clang-format enforced style and matches the coding standards.
> >
> > I'll give a citation to help: http://llvm.org/docs/CodingStandards.html#braced-initializer-lists
> Then clang-format should be fixed. That's what I used to verify I was correct. I just tried again and got the same result.
As noted above, we sorted this out...
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6955
@@ +6954,3 @@
+ // FIXME: Check if there is an encoding size reason to use PSHUFD when we
+ // don't pay the domain penalty.
+ return DAG.getNode(
----------------
Filipe Cabecinhas wrote:
> Nitpick: I would change the phrasing. If we only use one shufps, shufps is smaller (2 opcode bytes vs. 3 opcode bytes). If we need two shufps, then it's better to just use shufpd. The comment, as it is, seems, to me, like it suggests we still need to check if there's any case where this is helpful.
Not sure what you're really saying. First, we're talking about shufps vs. pshufd. Second, we would never need more or fewer of shufps vs. pshufd, they are both equally powerful when using a single input.
However, yes, I've now looked, and there is no reason for this. I'll nuke the FIXME.
If anything, we might want a combine somewhere *very* late that can see when we don't need the free copy pshufd provides and we don't pay a domain crossing penalty, and so it is an easy code size win to rewrite it as a shufps.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6965
@@ +6964,3 @@
+ SDLoc DL, SDValue V, MutableArrayRef<int> Mask,
+ const X86Subtarget *Subtarget, SelectionDAG &DAG) {
+ MutableArrayRef<int> LoMask = Mask.slice(0, 4);
----------------
Jim Grosbach wrote:
> Chandler Carruth wrote:
> > Jim Grosbach wrote:
> > > This is a really tricky part of the patch. Can you add a comment describing the approach and algorithm to the function? Without a lot of very explicit exposition, I'm concerned about future "improvements" to this code path inadvertently breaking things.
> > Yea, I didn't have that comment because there were many tens of different approaches tried before one worked. I'm also still hoping to simplify some parts of it.
> >
> > Also, all the details are already documented in each branch of the hybrid approach.
> Maybe something about "there be dragons here, and not the friendly kind" then? If it was that tricky to get right, I'm more concerned about someone helpfully coming along and "fixing" it later without understanding all the cases. Tests go a long way there, of course, but I don't like relying on that exclusively.
I've tried to do a decent job of commenting the strategy with appropriate warnings and cross references. I think it ended up being simpler to document than I feared, so hopefully its good now.
================
Comment at: lib/Target/X86/X86ISelLowering.cpp:7000
@@ +6999,3 @@
+ int DWordA = (ThreeInputHalfSum -
+ std::accumulate(ThreeInputs.begin(), ThreeInputs.end(), 0)) /
+ 2;
----------------
Jim Grosbach wrote:
> llvm/lib/Target/X86/X86ISelLowering.cpp:7000:24: error: no member named 'accumulate' in namespace 'std'
>
> Need to add "#include <numeric>" I think.
Done.
http://reviews.llvm.org/D4225
More information about the llvm-commits
mailing list