<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><br><div><blockquote type="cite"><div>On Jun 30, 2014, at 10:54 AM, Philip Reames <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>> wrote:</div><br class="Apple-interchange-newline"><div>
<meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type">
<div bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 06/30/2014 10:45 AM, Aditya
Nandakumar wrote:<br>
</div>
<blockquote cite="mid:D4B5E090-C98C-42E4-A899-A7A0538C391A@apple.com" type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
Yes - the key change is to add the operands.
<div>You are also right in that we could just add the operands to
the outer work list and get the same benefit. I was also
wondering why the sinking code is in InstCombine.</div>
<div>I have updated the patch to just add to the outer work list.</div>
</blockquote>
Thanks. The revised change is much more clear. <br>
<br>
With the change mentioned below, this LGTM. You should wait for
Chandler's signoff as well. <br>
<br>
Minor comments:<br>
- It looks like you can use the for-range loop here. for( auto op :
I->operands() ) {...}<br>
- Update your comment to emphasis the operands part. This was what
both I and Chandler didn't see at first in your original change
set. Something along the lines of "We'll add uses of the sunk
instruction below, but since sinking can expose opportunities for
it's *operands* add them to the worklist."<br>
- I'd structure the conditional as:<br>
if( TryToSinkInstruction(..) ) { MadeIRChange = true; for... };<br>
<br>
Philip<br>
<br>
<blockquote cite="mid:D4B5E090-C98C-42E4-A899-A7A0538C391A@apple.com" type="cite">
<div><br>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div><br>
<div>
<div>On Jun 27, 2014, at 4:23 PM, Philip Reames <<a moz-do-not-send="true" href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type">
<div bgcolor="#FFFFFF" text="#000000"> Just to make sure I
understand the issue properly, the key change here is
adding the *operands* rather than *users* to *a* worklist
right? We could potentially add them to the outer
worklist and get the same benefit?<br>
<br>
Looking through the code, we only seem to add the users of
a particular instruction to the worklist. For the sinking
code, it does make sense to reprocess the operands since
sinking the original instruction may have opened up
further opportunities. <br>
<br>
As a side note, I do find myself somewhat wondering why we
mix transforms and placement here. The addition of the
sinking code stands out in this loop. Anyone know why
this is done here rather than another pass? Just curious
about the history. <br>
<br>
Philip<br>
<br>
<div class="moz-cite-prefix">On 06/27/2014 01:17 PM,
Aditya Nandakumar wrote:<br>
</div>
<blockquote cite="mid:9C1E5AD9-D490-45DF-B951-9520F94F2B6D@apple.com" type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
Thanks Chandler for your review.
<div>The problem I saw in a few test cases was that
instcombine had finished visiting all instructions
(Iteration#0) and the only possible changes were
sinking of instructions. This sinking of one
instruction opened up sinking opportunities for other
instructions which were only being used by the
current(sunk) instruction. Since we are visiting
top-down, there is only one instruction being sunk per
iteration.</div>
<div>So in some my test cases, instcombine ran for 8
iterations where in iterations 1-8, it sank one
instruction per iteration. The test cases are about
150-300 lines long and we are visiting all those
instructions every iteration even though the only
change possible is the sinking.</div>
<div><br>
</div>
<div>Consider the following example. </div>
<div>bb1: %d0 = ...</div>
<div><span class="Apple-tab-span" style="white-space:pre"> </span>%d1 = .. %d0..</div>
<div>
<div><span class="Apple-tab-span" style="white-space:pre"> </span>%d2 = .. %d1..</div>
<div><span class="Apple-tab-span" style="white-space:pre"> </span>%d3 = op %d2 ..</div>
<div> <span class="Apple-tab-span" style="white-space:pre"> </span>...</div>
<div>bb2:</div>
<div> .. = op %d3</div>
</div>
<div><br>
</div>
<div>Currently instcombine would sink d3 in Iteration#0,
d2 in Iteration#1, d1 in Iteration#2 and d0 in
Iteration#3 - but it only requires one iteration in
all.</div>
<div><br>
</div>
<div>Updated patch - removed C style comments and fixed
typo(I think).</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div><br>
</div>
<div>
<div>
<blockquote type="cite">
<div>On Jun 26, 2014, at 5:54 PM, Chandler Carruth
<<a moz-do-not-send="true" href="mailto:chandlerc@google.com">chandlerc@google.com</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<div>
<div dir="ltr">I don't immediately understand
why this is such a performance improvement
here (and not elsewhere in instcombine)... Do
you have any insight into that? Is it a
locality problem that we don't re-visit the
other sinkable instructions in the right
order? It feels pretty weird to have a
worklist-inside-a-worklist, so I'm just trynig
to understand it better...
<div> <br>
</div>
<div>Also, the patch has a bunch of typos in
comments and uses C-style comments...</div>
</div>
<div class="gmail_extra"><br>
<br>
<div class="gmail_quote">On Mon, Jun 23, 2014
at 10:57 PM, Aditya Nandakumar <span dir="ltr"><<a moz-do-not-send="true" href="mailto:aditya_nandakumar@apple.com" target="_blank">aditya_nandakumar@apple.com</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px
#ccc solid;padding-left:1ex">Hi All<br>
<br>
I want to make a small optimization for
instcombine. While sinking instructions
(with single use) to the use block, also
check if any other instructions which are
used in the current instruction (operands)
can be sunk.<br>
<br>
This speeds up InstCombine time in several
cases as previously only one instruction
would be sunk per iteration.<br>
<br>
Please review<br>
<br>
Thanks<br>
<span class="HOEnZb"><font color="#888888">Aditya<br>
</font></span><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a moz-do-not-send="true" href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a moz-do-not-send="true" href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
llvm-commits mailing list
<a moz-do-not-send="true" class="moz-txt-link-abbreviated" href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>
<a moz-do-not-send="true" class="moz-txt-link-freetext" href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</div>
</div></blockquote></div><br></div></body></html>