<div class="gmail_quote">On Mon, Feb 23, 2009 at 3:10 PM, Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com">bob.wilson@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="Ih2E3d"><br>
On Feb 23, 2009, at 2:15 PM, Scott Michel wrote:<br>
> And the first thing the helper method would have to check is if this<br>
> SDNode is a BUILD_VECTOR node, right?<br>
<br>
</div>Right.  It's really not much different than what you have now, just<br>
moving the point where you check.  In your code right now, when you<br>
want to call your isConstantSplat method, you first dyn_cast the node<br>
to a BuildVectorSDNode.  Just move the check inside isConstantSplat.</blockquote><div><br>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?<br>
<br>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. <br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">There are a couple of different issues getting combined here.<br>
Changing the vector shuffles (as Nate is apparently already doing) is<br>
related in some ways to what you're doing with BUILD_VECTORS but it<br>
might be better to consider them as separate issues.  From what I can<br>
see, the point of your BuildVectorSDNode patch was simply to share<br>
code across target backends.  That doesn't have anything to do with<br>
vector shuffles.</blockquote><div><br>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.<br><br>
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.<br>
<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Once you remove the caching, there's not much left of<br>
BuildVectorSDNode, is there?  You might as well just add<br>
SDNode::isConstantSplat.</blockquote><div><br>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.<br>
<br>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.<br><br>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.<br>
<br>To summarize:<br>a) Shuffles are not constants. The two are very separate.<br>b) BuildVectorSDNode should probably become a ConstantVectorSDNode, similar to ConstantSDNode, et. al.<br>c) Killing the class breaks good O-O design.<br>
</div></div><br><br>-scooter<br>