<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 07/10/2014 04:12 PM, Chandler
Carruth wrote:<br>
</div>
<blockquote
cite="mid:CAGCO0Ki4SecWxUcjbCFq3+zGm+YUijdWwrYmPedeMzfuMp0MAw@mail.gmail.com"
type="cite">
<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>
</blockquote>
Chandler - Given what you said here, it really sounds like you're
not looking at an up to date version of the patch. There is now no
new worklist. <br>
<br>
Are you looking at Aditya's email from 6/30 10:45 am?<br>
<blockquote
cite="mid:CAGCO0Ki4SecWxUcjbCFq3+zGm+YUijdWwrYmPedeMzfuMp0MAw@mail.gmail.com"
type="cite">
<div dir="ltr">
<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 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">
<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 moz-do-not-send="true"
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>
<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
moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true"
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
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><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" target="_blank">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></fieldset>
<br>
<pre>_______________________________________________
llvm-commits mailing list
<a moz-do-not-send="true" href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>
<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>
</pre>
</blockquote>
<br>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a moz-do-not-send="true"
href="mailto:llvm-commits@cs.uiuc.edu"
target="_blank">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>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</body>
</html>