<div dir="ltr">Phab is slow/down, so sending email to list...<br><div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 18, 2015 at 7:57 PM, Gerolf Hoflehner <span dir="ltr"><<a href="mailto:ghoflehner@apple.com" target="_blank">ghoflehner@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The code looks pretty good, but I'd like to understand better why the new code investigates more patterns. <br></blockquote><div>... <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Allowing more than one pattern was part of the original design. What confused/confuses me is that in your old code you checked if operands had to be commuted in Root and Prev. But now the code only checks Root and potentially investigates two code sequences instead of one. Isn't that more expensive? And given that the order of the operands in Prev is not checked now, should there be a change in reassociateOps() addressing that?<br><div><div><br></div></div></blockquote><div><br></div><div>Yes, it's definitely more expensive to investigate two patterns rather than the one we did previously.<br></div><div><br><div>Before this patch, we would only match a 3 instruction sequence:<br></div><div>A = ? add ?<br></div><div>B = A add X<br></div><div>C = B add Y<br></div><div><br></div><div>So
 we could use the pattern match itself to determine if we need to 
commute the operands of the instruction that defines 'B' (aka 'Prev' in 
the code).<br><br></div><div>With this patch, we no longer assume anything
 about the instruction that defines 'A'. So we have to test both operands of Prev to see 
if there's a benefit to doing any reassociation. So for example, in the additional test case which has a fdiv: the pattern matcher does not know that one operand of Prev is being produced by a slow fdiv while the other is ready to go (the other operand is an input parameter to the function). <br><br>We could add code to the pattern matcher to look at 'A' again, but I think this would end up being exactly like the core logic in the MachineCombiner pass: we need to determine which operand of Prev is on the critical path and then decide if it's worth commuting the operands.<br><br></div><div>Here's the machine inst sequence that I'm referring to if that makes it any clearer:<br><br>    %vreg3<def> = COPY %XMM3; FR32:%vreg3<br>    %vreg2<def> = COPY %XMM2; FR32:%vreg2<br>    %vreg1<def> = COPY %XMM1; FR32:%vreg1<br>    %vreg0<def> = COPY %XMM0; FR32:%vreg0<br>    %vreg4<def> = VDIVSSrr %vreg0, %vreg1; FR32:%vreg4,%vreg0,%vreg1<br>    %vreg5<def> = VADDSSrr %vreg2, %vreg4<kill>; FR32:%vreg5,%vreg2,%vreg4<br>    %vreg6<def> = VADDSSrr %vreg3, %vreg5<kill>; FR32:%vreg6,%vreg3,%vreg5<br><br></div><div><br></div><div>Maybe there's some shortcut MI query that I can use to prune the search space? In this example, we have a copied operand (%vreg2) feeding into the first add. Would it be safe to simply ignore that possibility (ie, assume the copy does not add to the critical path)?<br></div><div><br><br><br><br></div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div><br><br> </div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10460&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=iMA3ODQTzIvCWhi4npCh1uCnin9_e4VpULQdXmgzz0k&s=usWAbJAvdLlSwbyHCak8IR_VuuXQ8hqlMje_bo-yVdw&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10460</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=iMA3ODQTzIvCWhi4npCh1uCnin9_e4VpULQdXmgzz0k&s=hZAP32Uiusn09Lgt8ajdfAc92hWg5b1hgQhIWyYI0wY&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</div></div></blockquote></div><br></div></div></div>