<div class="gmail_quote">On Mon, Feb 23, 2009 at 8:26 PM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com">clattner@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 style=""><br><div><div class="Ih2E3d"><div>On Feb 23, 2009, at 6:13 PM, Scott Michel wrote:</div><br><blockquote type="cite"><div class="gmail_quote">On Mon, Feb 23, 2009 at 4:03 PM, Nate Begeman <span dir="ltr"><<a href="mailto:natebegeman@me.com" target="_blank">natebegeman@me.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><br><div><div>It's basically as  Chris said; there will be a ShuffleVectorSDNode, and appropriate helper functions, node profile, and DAGCombiner support.</div>
</div></div></blockquote><div><br>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.)</div>
</div></blockquote><div><br></div></div><div>You're talking about two very very very different things: shuffle_vector "masks" and constant vectors.</div></div></div></blockquote><div><br>Yup. That's why I was absolutely mystified when you proposed the following:<br>
<br>2. Move the functionality of "is splat" etc to method somewhere, e.g. on SDNode.<br>
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).<br>
4. Move the helper functions from #2 back into ShuffleVectorSDNode.<br><br>#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.<br>
<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;"><div style=""><div><div></div><div></div><div>Constant vectors have to be legal for a target.  This has implications for legalization and many other things.  BUILD_VECTOR is fine for them.</div>
<div><br></div><div>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.</div>
</div></div></blockquote><div><br>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.<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;"><div style=""><div><div></div><div></div><div class="Ih2E3d"><blockquote type="cite">
<div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div><div></div><div>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.</div>
 </div></div></blockquote><div><br>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.</div>
</div></blockquote><div><br></div></div><div>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.</div>
</div></div></blockquote><div><br>The operands to shufflevector may be constant vectors, but that's not where the test for a constant splat ought to be.<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;">
<div style=""><div></div><div class="Ih2E3d"><div></div></div><div class="Ih2E3d"><blockquote type="cite"><div><br>b) BuildVectorSDNode should probably become a ConstantVectorSDNode, similar to ConstantSDNode, et. al.</div>
</blockquote><div><br></div></div><div>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).</div>
</div></blockquote><div><br>Keeping track of results is a bad thing? Ok.<br><br>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.<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;"><div style=""><div></div><div class="Ih2E3d"><blockquote type="cite"><div>c) Killing the class breaks good O-O design.<br>
</div></blockquote><div><br></div></div><div>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.</div>
</div></blockquote><div><br>buildvector never had anything to do with shuffles. The patch never had and never will. Somewhere along the lines I think you got confused.<br><br>
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.)<br><br><br>-scooter<br></div></div><br>