llvm dev - Feb 2009 - [LLVMdev] [llvm-commits] [llvm] r65296 - in /llvm/trunk: include/llvm/CodeGen/ lib/CodeGen/SelectionDAG/ lib/Target/CellSPU/ lib/Target/PowerPC/ lib/Target/X86/ test/CodeGen/X86/

On Feb 23, 2009, at 2:49 PM, Scott Michel wrote:
> On Mon, Feb 23, 2009 at 2:19 PM, Nate Begeman <natebegeman at me.com>
> wrote:
>
> On Feb 23, 2009, at 1:46 PM, Chris Lattner wrote:
>
> In my opinion, the proper direction for shuffles is:
>
> 1. Back out your patch.
> 2. Move the functionality of "is splat" etc to method somewhere,
e.g.
> on SDNode.
> 3. Introduce a new ShuffleVectorSDNode that only has two SDValue
> operands (the two input vectors), but that also contains an array of
> ints in the node (not as operands).
> 4. Move the helper functions from #2 back into ShuffleVectorSDNode.
>
> I'm working on #2 and #3 right now, and hope to land something in  
> the next couple days.
>
> Details, since this has implications for vector machine backends?
It's basically as  Chris said; there will be a ShuffleVectorSDNode,  
and appropriate helper functions, node profile, and DAGCombiner support.
> I'm just not groking how moving isSplat to SDNode eliminates  
> BUILD_VECTOR or deals with constant vector formation.
You can have a static method on SDNode that took an SDNode, checked if  
it was a build vector, and calculated whatever splat information you  
wanted.  There's no need for BuildVectorSDNode for this particular  
functionality.
> I like the idea of a ShuffleVectorSDNode (moves things in the  
> direction of a bijective ISD to SDNode mapping), but eliminating  
> BuildVectorSDNode in its entirety doesn't deal with constant and  
> variable vectors.
LLVM has no variable shuffle instruction, so there's no variable  
vectors to deal with; moving the constants into the shuffle sdnode  
does address constant shuffles (which are all of them).  Variable  
shuffles would need to be represented as target nodes at present, and  
my work will not change that.

Nate
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090223/7d291e83/attachment.html>

On Mon, Feb 23, 2009 at 4:03 PM, Nate Begeman <natebegeman at me.com>
wrote:
>
> It's basically as  Chris said; there will be a ShuffleVectorSDNode, and
> appropriate helper functions, node profile, and DAGCombiner support.
>
Fine. For vector shuffles. But again, what about vector constants, e.g.,
v4i32 <0, 1, 2, 3, 4>? BuildVectorSDNode is still a reasonable subclass to
have for encapsulating constant vectors (should be renamed, but hey, it's
what it's called today.)

You can have a static method on SDNode that took an SDNode, checked if
it
> was a build vector, and calculated whatever splat information you wanted.
>  There's no need for BuildVectorSDNode for this particular
functionality.
>
You're talking about moving the functionality to a class where it makes no
sense to look for it. That's one issue where I disagree and would argue for
good O-O software design. At least my patch puts the functionality in a
place where it's reasonable to expect it reside.

I like the idea of a ShuffleVectorSDNode (moves things in the direction of
a
> bijective ISD to SDNode mapping), but eliminating BuildVectorSDNode in its
> entirety doesn't deal with constant and variable vectors.
>
>
> LLVM has no variable shuffle instruction, so there's no variable
vectors to
> deal with; moving the constants into the shuffle sdnode does address
> constant shuffles (which are all of them).  Variable shuffles would need to
> be represented as target nodes at present, and my work will not change
that.
>
What about this case where the arguments are entirely variable and you have
no knowledge of their contents:

define <4 x i32> @v4i32_shuffle(<4 x i32> %a, <4 x i32> %b)
nounwind
readnone {
entry:
        %tmp1 = shufflevector <4 x i32> %a, <4 x i32> %b, <4 x
i32> < i32 1,
i32 2, i32 3, i32 0 >
        ret <4 x i32> %tmp1
}

It's perfectly legit LLVM code and it's a case where the two input
operands
are variables, not constants. Only the mask is a constant.


-scooter
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090223/c06751ae/attachment.html>

On Mon, Feb 23, 2009 at 4:03 PM, Nate Begeman <natebegeman at me.com>
wrote:
> I like the idea of a ShuffleVectorSDNode (moves things in the direction of
> a bijective ISD to SDNode mapping), but eliminating BuildVectorSDNode in
its
> entirety doesn't deal with constant and variable vectors.
>
>
> LLVM has no variable shuffle instruction, so there's no variable
vectors to
> deal with; moving the constants into the shuffle sdnode does address
> constant shuffles (which are all of them).  Variable shuffles would need to
> be represented as target nodes at present, and my work will not change
that.
>
I think what you meant to say here is that LLVM doesn't deal with
variable-length vectors and it does not deal with non-constant masks. That's
not the same what I was proposing, which is (now) an argument for O-O
hygiene and creating constant vectors.

Somehow I managed to get the impression that Chris was proposing moving
constant vector formation, e.g., v4i32 <0, 1, 2, 3, 4>, into the
ShuffleVectorSDNode class. That seems really odd, but that's how I read his
four steps. Happy to be proven that this was a misperception.

It's really in constant vector formation that isConstantSplat() is actually
useful. I'm not sure how useful isContantSplat() will be when moved to a
shuffle vector SDNode class.


-scooter
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090223/c7513557/attachment.html>

On Feb 23, 2009, at 6:13 PM, Scott Michel wrote:
> On Mon, Feb 23, 2009 at 4:03 PM, Nate Begeman <natebegeman at me.com>
> wrote:
>
> It's basically as  Chris said; there will be a ShuffleVectorSDNode,  
> and appropriate helper functions, node profile, and DAGCombiner  
> support.
>
> Fine. For vector shuffles. But again, what about vector constants,  
> e.g., v4i32 <0, 1, 2, 3, 4>? BuildVectorSDNode is still a reasonable
> subclass to have for encapsulating constant vectors (should be  
> renamed, but hey, it's what it's called today.)
You're talking about two very very very different things:  
shuffle_vector "masks" and constant vectors.

Constant vectors have to be legal for a target.  This has implications  
for legalization and many other things.  BUILD_VECTOR is fine for them.

Shuffle masks do not and should not be legalized as a build_vector.   
If you have a machine that has shuffles but has no support for forming  
a "buildvector" of a constant, you don't want to end up with  
shufflevectors of loads of the mask from the constant pool.

These are very different issues and should be modeled differently.
>
>
> You can have a static method on SDNode that took an SDNode, checked  
> if it was a build vector, and calculated whatever splat information  
> you wanted.  There's no need for BuildVectorSDNode for this  
> particular functionality.
>
> You're talking about moving the functionality to a class where it  
> makes no sense to look for it. That's one issue where I disagree and  
> would argue for good O-O software design. At least my patch puts the  
> functionality in a place where it's reasonable to expect it reside.
Moving these methods to SDNode was just a short term place to park  
them.  You could also just make them be global functions for all I  
care.  When Nate introduces a new shufflevector class in the next  
couple days  (apparently), we'd want to move these to that class.
> What about this case where the arguments are entirely variable and  
> you have no knowledge of their contents:
>
> define <4 x i32> @v4i32_shuffle(<4 x i32> %a, <4 x i32>
%b) nounwind
> readnone {
> entry:
>         %tmp1 = shufflevector <4 x i32> %a, <4 x i32> %b, <4
x i32>
> < i32 1, i32 2, i32 3, i32 0 >
>         ret <4 x i32> %tmp1
> }
>
> It's perfectly legit LLVM code and it's a case where the two input
> operands are variables, not constants. Only the mask is a constant.
Scott, we're only talking about the shuffle mask here.  Shuffle vector  
should always take two vectors as "operands".

> To summarize:
> a) Shuffles are not constants. The two are very separate.
Yes, we agree on this.
>
> b) BuildVectorSDNode should probably become a ConstantVectorSDNode,  
> similar to ConstantSDNode, et. al.
I'm fine with having a BuildVectorSDNode class in principle, but I  
don't like your current one for reasons I already mentioned (caching  
stuff that doesn't need to be etc).
> c) Killing the class breaks good O-O design.
buildvector should have nothing to do with shuffles.  Before your  
patch we didn't have "good OO design".  If you'd like to
improve OO
design, that is very welcome, but lets engineer it in a consistent  
direction.

-Chris



-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090223/db98ff02/attachment.html>