[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 1:20 PM, Scott Michel wrote:

> Chris:
>
> I did float this by the dev list first a couple of weeks ago, didn't  
> receive any comments.

Ok, I didn't see it, sorry about that.

> It's not entirely gratuitous; the rationale for adding a new node  
> class is threefold:
>
> a) Convenience for the backends. Since it benefits multiple backends  
> (PPC and CellSPU), it's a logical addition. I reckon the GPU efforts  
> would also benefit.

I don't see this.  Adding some helper methods would have the same  
functionality.

> b) Where else would one encapsulate a constant splat predicate?  
> SelectionDAG and SDNode are not good classes for constant splat  
> detection, since it's functionality specific to building vectors.

Eventually we need to add a ShuffleSDNode class for other reasons.   
Parking it on SelectionDAG or SDNode is not a good place to put it.

>
> c) Future work. At some point (or another), having target-specific  
> SDNode polymorphism is an issue that has to be addressed, in light  
> of the vector swizzling support conversation thread. One wants the  
> benefits of what the legalization and DAG combiner phases provide,  
> but not have to add more code for a target-specific ISD enumeration  
> value with a generic SDNode, if that functionality is already  
> available (and doesn't need to be overridden.)

This should be addressed in different ways.

I didn't go into it in the previous email, but I am strongly against  
several engineering issues in your patch.  "Remembering" whether  
something is a splat or not (for example) in the node is unnecessary  
and a really dangerous thing to do.  It is unnecessary because it can  
be computed on demand (as we do now) and it is dangerous, because it  
means that a) clients have to keep it up to date as they change the  
shuffle mask, and b) the nodes need to be reCSEd as the operands  are  
changed.

> I attempted to be pretty thorough with the change before committing.  
> I ran through the tests before committing. The changes were  
> primarily method invocations, which admittedly were pretty numerous.  
> It wasn't as if I was adding DebugLoc and changing prototypes  
> everywhere. :-)

I really appreciate your thoroughness and the fact that the patch  
apparently didn't break anything.

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.

The important part of #3 is that we really want an array of ints  
(using -1 for undef) for the shuffle mask, not "operands".  This  
eliminates the nastiness we have now were we need a buildvector, and  
it eliminates the dance we have to prevent the build vector from being  
legalized, and prevent the integer operands to it from being legalized.

Your patch doesn't get us further towards this eventual design point,  
which is why I prefer it to be reverted.

-Chris