[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 3:10 PM, Bob Wilson <bob.wilson at apple.com> wrote:

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


I got that. LLVM will need to support _some class_ that encapsulates
constant vectors. It might suffice to say that at this point, what is
ISD::BUILD_VECTOR should really become the equivalent of ISD::Constant for
vectors. I could refine my patch so that this particular functionality is
implemented. But I'm really not convinced that moving a constant splat
predicate to SDNode is good O-O software design. Really - Is SDNode the
right place to look for a constant splat predicate?

dyn_cast is generally unavoidable when you need to get at the subclass, i.e.
ConstantSDNode and any other node subclassed off SDNode. I'm not entirely
buying that as rationale for killing the class and moving the predicate.

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.


Part of the reason was consolidating code, partially for the convenience of
the CellSPU backend where a lot of stuff now gets "lowered" during
instruction selection.

At a larger perspective, shuffles and vector constants are two separate
issues. I'm not clear why shuffles and constants were combined into the same
thread. I've never proposed that shuffles and vector constants should be
combined.

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


No, that's not true: you lose an important feature of O-O software design,
namely, encapsulation of the results of the constant splat predicate.
isConstantSplat() produces a number of results, e.g., splat size, the actual
splat bits. These would still be generated on the fly, even if the result of
the previous invocation were no longer cached.

Which means, that if one moves the functionality to SDNode, all of these
results have to be passed by reference. That's generally the hallmark of
not-so-desirable O-O software design.

I'm not saying we need to turn LLVM into the paragon of O-O-ness. But let's
use it when it's advantageous.

To summarize:
a) Shuffles are not constants. The two are very separate.
b) BuildVectorSDNode should probably become a ConstantVectorSDNode, similar
to ConstantSDNode, et. al.
c) Killing the class breaks good O-O design.


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