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 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>

On Mon, Feb 23, 2009 at 8:26 PM, Chris Lattner <clattner at apple.com>
wrote:
>
> 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.
>
Yup. That's why I was absolutely mystified when you proposed the following:

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.

#4 is the part that completely mystifies me. I reckon that I could almost
live with #2, as much I see it as being unsound software engineering and
more of a performance optimization.


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.
>
Never disagreed with that point. But recall what you had previous written
and quoted above. Hence my confusion when you wrote step #4 -- that's an
even less logical place to look for isConstantSplat functionality and is
really misplaced.

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.
>
The operands to shufflevector may be constant vectors, but that's not where
the test for a constant splat ought to be.

> 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).
>
Keeping track of results is a bad thing? Ok.

Removing the computedSplat bool is easy. And I could add the caveat that
isConstantSplat should be called before any of the result state getter
methods are called. I'm not sure that passing a bunch of references to a
function is any more wholesome.

> 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.
>
buildvector never had anything to do with shuffles. The patch never had and
never will. Somewhere along the lines I think you got confused.

But hey, you're the boss. Will rip the patch out at earliest opportunity and
live with the consequences, even if testing for constant splats ends up in a
shufflevector class (see #4 above.)


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

On Feb 23, 2009, at 10:30 PM, Scott Michel wrote:
> On Mon, Feb 23, 2009 at 8:26 PM, Chris Lattner <clattner at
apple.com>
> wrote:
>
> 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.
>
> Yup. That's why I was absolutely mystified when you proposed the  
> following:
>
> 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.
>
> #4 is the part that completely mystifies me. I reckon that I could  
> almost live with #2, as much I see it as being unsound software  
> engineering and more of a performance optimization.
I don't see how it is relevant.  The representation of constants and  
splat masks should be *complete different*.  If you want to see if a  
shuffle mask is a splat and/or if a constant vector is a splat, then  
you'd need different code for both.
>> 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.
>
> buildvector never had anything to do with shuffles. The patch never  
> had and never will. Somewhere along the lines I think you got  
> confused.
Entirely possible, that frequently happens! :)
> But hey, you're the boss. Will rip the patch out at earliest  
> opportunity and live with the consequences, even if testing for  
> constant splats ends up in a shufflevector class (see #4 above.)
To be clear, I'm entirely in favor of improving support for *both*  
buildvector and shuffle.  Most of my comments have been about shuffle,  
but improvements to introduce a new buildvectorsdnode class would also  
make sense... but they should be separate from shuffle stuff.

Thanks Scott!

-Chris

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