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/

Chris:

I did float this by the dev list first a couple of weeks ago, didn't receive
any comments. 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.
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.
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.)

It seems to me that LLVM will eventually need to have a bijective mapping
between the ISD enumeration and the SDNodes, in the case of (c). (c) is a
huge undertaking and needs careful thought. For example, the whole "Is the
node legal, custom, promotable or expanded" testing screams for static
protocol methods hooked to per-target protected virtual methods.

But (a) and (b) are the dominant factors for contributing the change. It's
not completely altruistic: the commit makes it considerably easier for the
CellSPU backend to deal with various splats at instruction selection rather
than during target-dependent lowering.

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. :-)

Ok, I'm sure there were places in clang and llvm-gcc-4.2 that I overlooked,
and I appologize for being narrowly focused on the backend infrastructure.


-scooter

On Mon, Feb 23, 2009 at 12:49 PM, Chris Lattner <clattner at apple.com>
wrote:
> fyi in case you're not reading commits.
>
> Begin forwarded message:
>
>  From: Chris Lattner <clattner at apple.com>
>> Date: February 23, 2009 12:48:33 PM PST
>> To: Commit Messages and Patches for LLVM <llvm-commits at
cs.uiuc.edu>
>> Subject: Re: [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/
>> Reply-To: Commit Messages and Patches for LLVM <llvm-commits at
cs.uiuc.edu>
>>
>>
>> On Feb 22, 2009, at 3:36 PM, Scott Michel wrote:
>>
>>  Author: pingbak
>>> Date: Sun Feb 22 17:36:09 2009
>>> New Revision: 65296
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=65296&view=rev
>>> Log:
>>> Introduce the BuildVectorSDNode class that encapsulates the
>>> ISD::BUILD_VECTOR
>>> instruction. The class also consolidates the code for detecting
>>> constant
>>> splats that's shared across PowerPC and the CellSPU backends
(and
>>> might be
>>> useful for other backends.) Also introduces
>>> SelectionDAG::getBUID_VECTOR() for
>>> generating new BUILD_VECTOR nodes.
>>>
>>
>> Hi Scott,
>>
>> Before making a major change like this it really is best to float the
>> idea by the list.
>>
>> I don't see any reason to make this sort of change.  Sharing code
>> between targets is definitely goodness, but the way you are doing this
>> seems very strange.  Instead of creating a new SDNode class, why not
>> move the various functionality to helper functions on SDNode or
>> SelectionDAG?
>>
>> -Chris
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090223/5b89fcc6/attachment.html>

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

On Mon, Feb 23, 2009 at 1:46 PM, Chris Lattner <clattner at apple.com>
wrote:
>
> 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 happens. :-)

> 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.
>>
>
> It's not entirely gratuitous; the rationale for adding a new node class
is
> threefold:
>
> I don't see this.  Adding some helper methods would have the same
> functionality.

And the first thing the helper method would have to check is if this SDNode
is a BUILD_VECTOR node, right?

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

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

In my opinion, the proper direction for shuffles is:
>
> 1. Back out your patch.

Before I back it out, please read my comments below carefully. :-)

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

There are two cases: a constant vector and a vector shuffle (moving two
vectors around with a constant mask, which itself is a constant vector.) I'm
not seeing how a splat is equivalent to a shuffle (unless the second input
operand is totally undef and the mask has a magic pattern, which I think
confuses more than clarifies.) On a pure vector machine, like the CellSPU,
splats and shuffles are not equivalent concepts (*)

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.

Agreed, and I can see why one would want to have an array of <something>
instead of operands to build the shuffle mask. The mask is generally a byte
array, at least for CellSPU and PPC, but could be a bitmask (cell has one of
those too.) I think you're still left with a constant formation problem in
the case of a constant vector that is separate from vector shuffles.

Incidentally, -1 is a perfectly legal value for a CellSPU shuffle mask in
the byte array (actually, it's 0b1xxxxxxxx, but that doesn't mean it
can't
or won't occur.)


-scooter

(*) To be fair, shuffles are part of i64 constant formation on CellSPU for
special values of the mask.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20090223/5083bde7/attachment.html>

On Feb 23, 2009, at 1:46 PM, Chris Lattner wrote:
> 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.
I'm working on #2 and #3 right now, and hope to land something in the  
next couple days.

Nate

> 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).
...
> 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.
This is PR2957 (which originally suggested a variadic SDNode, but it
quickly became clear that an array of ints is better).  It would be
great to have a volunteer for this (I don't have time).

Ciao,

Duncan.