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