<div dir="ltr">Sorry it's taken me a while to get back to this.<div><br></div><div>I looked at the patch, and I'm still feeling like there has to be a more direct way to use the instcombine worklist to address this problem rather than adding another worklist on top. Having a feeling isn't a very good code review though. =D So I grabbed your patch and tried to build a benchmark just as you suggested to see what the performance difference was and whether there was another way to achieve it.</div>
<div><br></div><div>However, despite building a benchmark that sinks some 400 instructions, I can measure no performance improvement with your patch... I'm beginning to suspect that my test case may just not be representative, could you attach a (compressed if needed) test case that actually shows the performance difference so that I can experiment with other approaches?</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jul 2, 2014 at 10:49 AM, Aditya Nandakumar <span dir="ltr"><<a 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"><div style="word-wrap:break-word">Hi Chandler,<div><br></div><div>Could you please look through the patch?</div><div><br>
</div><div>Thanks</div><div>Aditya<br><div><blockquote type="cite"><div><div class="h5"><div>On Jun 30, 2014, at 11:09 AM, Aditya Nandakumar <<a href="mailto:aditya_nandakumar@apple.com" target="_blank">aditya_nandakumar@apple.com</a>> wrote:</div>
<br></div></div><div><div><div class="h5"><div style="word-wrap:break-word">Thanks Philip<div><br></div><div>I have updated the patch with your suggested changes. Waiting for chandler’s review.</div><div><br></div><div>Thanks</div>
<div>Aditya</div><div></div></div></div></div><span><InstCombineUpdated.patch></span><div><div class="h5"><div style="word-wrap:break-word"><br><div><blockquote type="cite"><div>On Jun 30, 2014, at 10:54 AM, Philip Reames <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>> wrote:</div>
<br><div>
<div bgcolor="#FFFFFF" text="#000000">
<br>
<div>On 06/30/2014 10:45 AM, Aditya
Nandakumar wrote:<br>
</div>
<blockquote type="cite">
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 type="cite">
<div><br>
</div>
<br>
<fieldset></fieldset>
<br>
<div><br>
<div>
<div>On Jun 27, 2014, at 4:23 PM, Philip Reames <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>>
wrote:</div>
<br>
<blockquote type="cite">
<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>On 06/27/2014 01:17 PM,
Aditya Nandakumar wrote:<br>
</div>
<blockquote type="cite">
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 style="white-space:pre-wrap"> </span>%d1 = .. %d0..</div>
<div>
<div><span style="white-space:pre-wrap"> </span>%d2 = .. %d1..</div>
<div><span style="white-space:pre-wrap"> </span>%d3 = op %d2 ..</div>
<div> <span style="white-space:pre-wrap"> </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></fieldset>
<br>
<div><br>
</div>
<div>
<div>
<blockquote type="cite">
<div>On Jun 26, 2014, at 5:54 PM, Chandler Carruth
<<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>>
wrote:</div>
<br>
<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 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><font color="#888888">Aditya<br>
</font></span><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a 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></fieldset>
<br>
<pre>_______________________________________________
llvm-commits mailing list
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">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>
_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></div></blockquote></div><br></div></div></blockquote></div><br></div>