<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><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>Some random comments:</div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+  /// 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; font: normal normal normal 12px/normal Inconsolata; ">+  /// 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; font: normal normal normal 12px/normal Inconsolata; ">+  /// 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><br></div><div>Also, the way it is used is confusing.  Why not name it "getSplatElement" and have it return null on failure?</div><div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+Value *Value::getThruSplat() {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 12px/normal Inconsolata; ">+  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><br></div><div>Otherwise, looks fine to me.</div><div><br></div><div>-Chris</div></div><br></body></html>