<div class="gmail_quote">On 29 October 2010 21:34, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com">clattner@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

<div style="word-wrap:break-word"><div class="im"><div><div>On Oct 29, 2010, at 7:28 PM, Nick Lewycky wrote:</div><blockquote type="cite"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Which reminds me; your current patch doesn't handle vector ConstantAggregateZero.<br></blockquote>
<div>
<br></div><div>Good catch! Updated patch attached.</div></div></blockquote><br></div></div><div>Some random comments:</div><div><br></div><div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

+  /// getThruSplat - This method returns the singular splat value held by this</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+  /// value if it is a ConstantVector holding a splat, otherwise it returns</div>

<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+  /// itself.</div><div><br></div><div>This description is confusing to me.  How about something like:</div><div><br></div><div>If the current value is a constant vector where all of the elements have the same value, return that value.  Otherwise, return the current value.</div>

</div></div></blockquote><div><br></div><div>Much better. Thanks!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div style="word-wrap:break-word">
<div>
<div>Also, the way it is used is confusing.  Why not name it "getSplatElement" and have it return null on failure?</div></div></div></blockquote><div><br></div><div>Because I want to preserve the pattern of a declaring a single variable inside an if-statement with a dyn_cast. I'm planning to find and update code which currently does that with a test for ConstantInt/FP and don't want to add lots of this pattern:</div>

<div><br></div><div>  Value *V = value;</div><div>  if (ConstantVector *CV = dyn_cast<ConstantVector>(V))</div><div>    if (Constant *Splat = CV->getSplatValue())</div><div>      V = Splat;</div><div>  if (ConstantInt *CI = dyn_cast<ConstantInt>(V)) {</div>

<div>    ...</div><div><br></div><div>all over. If getSplatElement() returned NULL instead of "this" then I would only be able to remove one line of that (the test for ConstantVector).</div><div><br></div><div>
I guess I don't really understand what makes the replacement code confusing.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div style="word-wrap:break-word"><div><div>+Value *Value::getThruSplat() {</div><div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

+  switch (getValueID()) {</div></div><div><br></div><div>This would be more natural and elegant written with two dyncasts instead of a switch.  The premature optimization isn't worth it, and llvm at least will turn the two dyncasts into a switch anyway.</div>

</div></div></blockquote><div><br></div><div>Okay!</div><div><br></div><div>Nick</div><div><br></div></div>