<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 15, 2017 at 8:11 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"><div><div class="h5">
<p><br>
</p>
<div class="m_2069289546685357480moz-cite-prefix">On 09/15/2017 09:59 PM, Xinliang David
Li wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Fri, Sep 15, 2017 at 7:47 PM, Hal
Finkel via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">hfinkel
added inline comments.<br>
<br>
<br>
================<br>
<span>Comment at: lib/Transforms/InstCombine/Ins<wbr>tCombinePHI.cpp:72<br>
+// int_ptr = BitCast(ptr_ptr)<br>
+// int_init = Load(int_ptr)<br>
+// br label %bb2<br>
----------------<br>
davidxl wrote:<br>
> davidxl wrote:<br>
> > hfinkel wrote:<br>
> > > I don't understand why you're looking for
a load here specifically. It's a good example, because
it shows that you might need to look for something here
other than a ptrtoint, but that seems to motivate taking
a general input (not specifically looking for this load
as you do below).<br>
> > Checking for loads because instcombine can
fold the inserted inttoptr away. Same for PHI defs. For
other instructions, we are basically just hoisting
inttoptr above to the PHI operand, which is unclear it
is a win.<br>
> Also general input is already handled first -- by
looking in forward direction if there is an existing
inttoptr that can be reused.<br>
><br>
> bb1:<br>
> %arg_int64 = <general operation><br>
> ...<br>
> %ptr_val = inttoptr i64 %arg_int64 to float*<br>
><br>
> ...<br>
> bbm:<br>
> %int64_phi = PHI ([%arg_int64, %bb1], [...])<br>
> %ptr_val = inttoptr i64 %int64_phi to float*<br>
><br>
> ==><br>
><br>
> %ptr_val = PHI([%ptr_val, %bb1], [...])<br>
><br>
><br>
><br>
> Checking for loads because instcombine can fold the
inserted inttoptr away. Same for PHI defs. For other
instructions, we are basically just hoisting inttoptr
above to the PHI operand, which is unclear it is a win.<br>
<br>
</span>Seems like a win to me. It will make AA more
powerful in the loop.</blockquote>
<div><br>
</div>
<div><br>
</div>
<div>In fact, my previous version of the patch does that --
it actually checks if we can hoist intptr out of the loop.
Unfortunately it does not work well --> for some
reason, the LoopInfo is not properly updated, so I could
not eliminate the intptr properly which puzzled me for a
while. It could be a bug with the new PM, but I did not
have time to track it down.</div>
<div><br>
</div>
<div>For now I prefer handling the limited case first and
follow up with more general cases when the need arises.
Doing this incrementally is also less risky. Does it sound
fine to you?</div>
</div>
</div>
</div>
</blockquote>
<br></div></div>
I don't understand why any of these things would need to update
LoopInfo: The only state that LoopInfo keeps is the list of blocks
in the loop and the list of subloops. Neither of which this changes.
If your more-general patch triggers a bug with LoopInfo, wouldn't
this one too (just less often because the transformation triggers
less often)?<br></div></blockquote><div><br></div><div><br></div><div>I wasn't clear. What I meant that the LoopInfo passed to the InstCombine is already incorrect. The issue does not exist when running instcombine in isolation with opt, but shown with more complicated pipelines. That is certainly an orthogonal issue.</div><div><br></div><div>David</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000">
<br>
Thanks again,<br>
Hal<span class=""><br>
<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div><br>
</div>
<div><br>
</div>
<div>thanks,</div>
<div><br>
</div>
<div>David</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Besides, we can have a separate combine for<br>
inttoptr(load(bitcast(pp))) => load(pp)<br>
<br>
</blockquote>
<div><br>
</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
(assuming we can prove what's necessary to make AA on
these pointers safe)<br>
<br>
<br>
<a href="https://reviews.llvm.org/D37832" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3783<wbr>2</a><br>
<br>
<br>
<br>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</span><span class="HOEnZb"><font color="#888888"><pre class="m_2069289546685357480moz-signature" cols="72">--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory</pre>
</font></span></div>
</blockquote></div><br></div></div>