<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><br></div><div><br><div><div>On Sep 7, 2012, at 4:16 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><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>
</blockquote></div><br></div></body></html>