<div class="gmail_extra"><div class="gmail_quote">On Fri, Sep 7, 2012 at 5:24 PM, Owen Anderson <span dir="ltr"><<a href="mailto:resistor@mac.com" target="_blank" class="cremed">resistor@mac.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">Chandler,<div><br></div><div>We can't write a test case to exercise specifically this path, because it is not an observable change on any in-tree targets, since the DAG combiner also performs the same transform. Â Observing the difference between the two requires specific interactions with target-specific DAG combine's. Â The best we could do would be to write a test that will exercise whichever path happens to get run, but it will <i>not</i> break if this code gets broken.</div>
<div><br></div><div>That said, it's still worthwhile from a compile time perspective to aggressively prevent the creation of new SDNode's (with all their associated baggage) rather than leaving it up to the DAG combiner. Â You'll notice that the code this patch touches already does the same thing for FADD and FSUB.</div>
</div></blockquote><div><br></div><div>Oh sure, it just wasn't clear that the dag combiner already had code for this from the initial email. =] Maybe include a note about that in the commit log.</div><div><br></div><div>
<insert pining over the lack of a unittest framework for the selection DAG that would allow us to write such a test></div><div><br></div><div>Tiny, minor patch comment that doesn't really matter. LGTM in general.</div>
<div><br></div><div><div>+ Â  Â  Â } else if (Opcode == ISD::FMUL) {</div><div>+ Â  Â  Â  Â if (ConstantFPSDNode *CFP = dyn_cast<ConstantFPSDNode>(N1)) {</div><div>+ Â  Â  Â  Â  Â // 0*x --> 0</div><div>+ Â  Â  Â  Â  Â if (CFP->isZero())</div>
<div>+ Â  Â  Â  Â  Â  Â return N1;</div><div>+ Â  Â  Â  Â  Â // 1*x --> x</div><div>+ Â  Â  Â  Â  Â if (CFP->isExactlyValue(1.0))</div><div>+ Â  Â  Â  Â  Â  Â return N2;</div><div>+ Â  Â  Â  Â }</div><div>+ Â  Â  Â  Â if (ConstantFPSDNode *CFP = dyn_cast<ConstantFPSDNode>(N2)) {</div>
<div><br></div><div>Rather than duplicating the logic you could write:</div><div><br></div><div>ConstantFPSDNode *CFP = dyn_cast<ConstantFPSDNode>(N1)</div><div>if (!CFP)</div><div>  CFP = dyn_cast<ConstantFPSDNode>(N2)</div>
<div>if (CFP) {</div><div>  // transform logic</div><div>}</div></div></div></div>