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

[Scott Michel]

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.


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

Never disagreed with that point. But recall what you had previous written
and quoted above. Hence my confusion when you wrote step #4 -- that's an
even less logical place to look for isConstantSplat functionality and is
really misplaced.

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

The operands to shufflevector may be constant vectors, but that's not where
the test for a constant splat ought to be.


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

Keeping track of results is a bad thing? Ok.

Removing the computedSplat bool is easy. And I could add the caveat that
isConstantSplat should be called before any of the result state getter
methods are called. I'm not sure that passing a bunch of references to a
function is any more wholesome.


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

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


-scooter
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20090223/6e90daa7/attachment.html>