<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Feb 23, 2009, at 10:30 PM, Scott Michel wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><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.</div></div></blockquote><div><br></div><div>I don't see how it is relevant. The representation of constants and splat masks should be *complete different*. If you want to see if a shuffle mask is a splat and/or if a constant vector is a splat, then you'd need different code for both.</div><br><blockquote type="cite"><div class="gmail_quote"><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 class="Ih2E3d"><blockquote type="cite"><div><span class="Apple-style-span" style="-webkit-text-stroke-width: -1; ">c) Killing the class breaks good O-O design.</span></div></blockquote></div></div></blockquote><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 class="Ih2E3d"><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.</div></div></blockquote><div><br></div>Entirely possible, that frequently happens! :)</div><div><br><blockquote type="cite"><div class="gmail_quote"><div>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></div></div></blockquote></div><br><div>To be clear, I'm entirely in favor of improving support for *both* buildvector and shuffle. Most of my comments have been about shuffle, but improvements to introduce a new buildvectorsdnode class would also make sense... but they should be separate from shuffle stuff.</div><div><br></div><div>Thanks Scott!</div><div><br></div><div>-Chris</div><div><br></div></body></html>