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

[Chris Lattner]

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>