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

[Bob Wilson]

On Feb 23, 2009, at 2:15 PM, Scott Michel wrote:
> And the first thing the helper method would have to check is if this  
> SDNode is a BUILD_VECTOR node, right?

Right.  It's really not much different than what you have now, just  
moving the point where you check.  In your code right now, when you  
want to call your isConstantSplat method, you first dyn_cast the node  
to a BuildVectorSDNode.  Just move the check inside isConstantSplat.

>
>
> 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.
>
> Ok, so that's a subclass of BuildVectorSDNode or the ShuffleSDNode  
> class is passed one or more BuildVectorSDNode operands (vide infra.)

There are a couple of different issues getting combined here.   
Changing the vector shuffles (as Nate is apparently already doing) is  
related in some ways to what you're doing with BUILD_VECTORS but it  
might be better to consider them as separate issues.  From what I can  
see, the point of your BuildVectorSDNode patch was simply to share  
code across target backends.  That doesn't have anything to do with  
vector shuffles.

>
> 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 can take the result caching out, seemed to be a good idea at the  
> time. The predicate is only called in a couple of places and I was  
> prematurely optimizing for the case when it's called repeatedly.
>
> However, (b) is something that is handled elsewhere.  
> BuildVectorSDNode is merely a container class -- the constructor  
> isn't doing anything funky other than calling the base constructor  
> with the appropriate SDOperand list and size. I seem to recall that  
> calling UpdateNodeOperands is generally frowned upon, but I'm happy  
> to look into that particular issue. But at this point, I didn't see  
> any place where a BUILD_VECTOR has its operands updated. Removing  
> the result caching would avoid bugs, with which I agree.

Once you remove the caching, there's not much left of  
BuildVectorSDNode, is there?  You might as well just add  
SDNode::isConstantSplat.