<div dir="ltr">I have gone ahead and created a request to merge this into 3.9.1, I hope that's okay.<div><br></div><div><a href="https://llvm.org/bugs/show_bug.cgi?id=30307">https://llvm.org/bugs/show_bug.cgi?id=30307</a><br></div><div><br></div><div>/Chris</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 31, 2016 at 5:00 PM, Chris Sakalis <span dir="ltr"><<a href="mailto:chrissakalis@gmail.com" target="_blank">chrissakalis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Great, thank you!</div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 31, 2016 at 2:07 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><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:#000000"><br><hr><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><b>From: </b>"Chris Sakalis" <<a href="mailto:chrissakalis@gmail.com" target="_blank">chrissakalis@gmail.com</a>><br><b>To: </b>"Daniel Berlin" <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>><br><b>Cc: </b>"Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>>, "David Majnemer" <<a href="mailto:david.majnemer@gmail.com" target="_blank">david.majnemer@gmail.com</a>>, "llvm-dev" <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>, "Philip Reames" <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>>, "Davide Italiano" <<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>>, "Chandler Carruth" <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>><br><b>Sent: </b>Wednesday, August 31, 2016 3:58:56 AM<span><br><b>Subject: </b>Re: [llvm-dev] GVN / Alias Analysis issue with llvm.masked.scatter/gather intrinsics<br><br></span><span><div dir="ltr">Thank you for the quick fix, I can no longer reproduce the issue. As far a releases go, I am guessing that this is going to be in 4.0?</div></span></blockquote>Yes, and we can consider it for 3.9.1 as well.<span><font color="#888888"><br><br> -Hal</font></span><div><div><br><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div><br></div><div>Best,</div><div><br></div><div>Chris<br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 30, 2016 at 9:26 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Yeah, i just hope it doesn't regress scatter/gather vector code badly.<div>But at least it's correct now?<br><br></div></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 30, 2016 at 1: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:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div style="font-family:arial,helvetica,sans-serif;font-size:10pt;color:rgb(0,0,0)"><br><hr><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><b>From: </b>"Daniel Berlin" <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>><br><b>To: </b>"Philip Reames" <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>>, "Davide Italiano" <<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>>, "Chandler Carruth" <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>><br><b>Cc: </b>"Chris Sakalis" <<a href="mailto:chrissakalis@gmail.com" target="_blank">chrissakalis@gmail.com</a>>, "David Majnemer" <<a href="mailto:david.majnemer@gmail.com" target="_blank">david.majnemer@gmail.com</a>>, "Hal Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>>, "llvm-dev" <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>><br><b>Sent: </b>Tuesday, August 30, 2016 3:07:01 PM<br><b>Subject: </b>Re: [llvm-dev] GVN / Alias Analysis issue with llvm.masked.scatter/gather intrinsics<span><br><br><div dir="ltr">This is now committed and a test added to GVN.<div><br></div></div></span></blockquote>Thanks!<br><br>I suspect that, in practice, we'll get little benefit from handling this until our AA passes learn how to deal with (i.e. look back through) pointer vectors.<br><br> -Hal<div><div><br><blockquote style="border-left:2px solid rgb(16,16,255);margin-left:5px;padding-left:5px;color:rgb(0,0,0);font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div><div><br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 29, 2016 at 1:59 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Okay, so then it sounds like, for now, the right fix is to stop marking masked.gather and masked.scatter with intrarg* options.<div><div><div><br><div class="gmail_quote"><div dir="ltr">On Mon, Aug 29, 2016, 1:26 PM Philip Reames <<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p>We might have specification bug here, but we appear to implement
what we specified. argmemonly is specified as only considering
pointer typed arguments. It's behavior on vector-of-pointers is
unspecified, but would seem to fall into the same case as inttoptr
of an integer argument (i.e. explicitly undefined). We could
consider changing that.<br>
</p></div><div>
<p><br>
</p>
<br>
<div>On 08/29/2016 11:59 AM, Daniel Berlin
wrote:<br>
</div>
<blockquote>
<div dir="ltr">+ a few others.
<div><br>
</div>
<div>After following this rabbit hole a bit, there are a lot of
mutually recursive calls, etc, that may or may not do the
right thing with vectors of pointers.</div>
<div>I can fix *this* particular bug with the attached patch.</div>
<div><br>
</div>
<div>However, it's mostly papering over stuff. Nothing seems to
know what to do with a memorylocation that is a vector of
pointers. They all expect memorylocation to be a single
pointer location.</div>
<div><br>
I would chalk it up to "luck" that this patch fixes the bug.</div>
<div><br>
</div>
<div>It's pretty clear that MemoryLocation doesn't fit the needs
of a lot of stuff anymore (we hacked AA nodes into it, and
lots of stuff now tries to figure out the invariantess of the
locations, blah blah blah), but it seems like a big job to
figure out what to replace it with that will work for these
cases.</div>
<div><br>
</div>
<div>(I'm pretty positive if we just make it MemoryLocations,
and have everything loop over the locations, the compiler will
get a lot larger and a lot slower)</div>
</div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Mon, Aug 29, 2016 at 9:10 AM, Daniel
Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">it would also, for that matter, say the same
about an array of pointers.
<div><br>
</div>
<div>It's not clear to me what will break if you change
this to isPtrOrPtrVectorTy.</div>
<div><br>
</div>
<div>In fact, i know it doesn't fix this bug.</div>
<div>It's a pretty deep rabbit hole of things not quite
prepared to understand vectors of pointers.</div>
<div><br>
</div>
<div>(we prepare memorylocations of them, but memory
locations expect to be one thing, not a group of things,
etc).</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
</div>
<div>
<div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Mon, Aug 29, 2016 at 8:58
AM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">this is definitely a bug in AA.
<div><br>
</div>
<div>
<div> 225 <span style="white-space:pre-wrap"> </span>
for (auto I = CS2.arg_begin(), E =
CS2.arg_end(); I != E; ++I) {</div>
<div> 226 <span style="white-space:pre-wrap"> </span>
const Value *Arg = *I;</div>
<div> 227 <span style="white-space:pre-wrap"> </span>
if
(!Arg->getType()->isPointerTy(<wbr>))</div>
<div>-> 228 <span style="white-space:pre-wrap"> </span>
continue;</div>
<div> 229 <span style="white-space:pre-wrap"> </span>
unsigned CS2ArgIdx =
std::distance(CS2.arg_begin(), I);</div>
<div> 230 <span style="white-space:pre-wrap"> </span>
auto CS2ArgLoc =
MemoryLocation::getForArgument<wbr>(CS2,
CS2ArgIdx, TLI);</div>
</div>
<div><br>
</div>
<div> AliasAnalysis.cpp:228<br>
</div>
<div><br>
</div>
<div>It ignores every argument because they are
vectors of pointers, not pointers.</div>
<div><br>
</div>
<div><br>
</div>
<div>I'm surprised this has not broken anything
before. It will never say two intrinsics with
vectors of pointers mod/ref each other.</div>
<div><br>
</div>
</div>
<div>
<div>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Mon, Aug 29,
2016 at 7:36 AM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+ Daniel
Berlin<br>
<div>
<div><br>
On Mon, Aug 29, 2016 at 3:42 PM,
Chris Sakalis via llvm-dev<br>
<<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>
wrote:<br>
> Hello everyone,<br>
><br>
> I think I have found an gvn /
alias analysis related bug, but
before opening<br>
> an issue on the tracker I
wanted to see if I am missing
something. I have<br>
> the following testcase:<br>
><br>
>> define spir_kernel void
@test(<2 x i32*> %in1, <2 x
i32*> %in2, i32* %out)<br>
>> {<br>
>> entry:<br>
>> ; Just some temporary
storage<br>
>> %tmp.0 = alloca i32<br>
>> %tmp.1 = alloca i32<br>
>> %tmp.i = insertelement
<2 x i32*> undef, i32* %tmp.0,
i32 0<br>
>> %tmp = insertelement
<2 x i32*> %tmp.i, i32*
%tmp.1, i32 1<br>
>> ; Read from in1 and in2<br>
>> %in1.v = call <2 x
i32>
@llvm.masked.gather.v2i32(<2 x
i32*> %in1, i32<br>
>> 1, <2 x i1> <i1
true, i1 true>, <2 x i32>
undef) #1<br>
>> %in2.v = call <2 x
i32>
@llvm.masked.gather.v2i32(<2 x
i32*> %in2, i32<br>
>> 1, <2 x i1> <i1
true, i1 true>, <2 x i32>
undef) #1<br>
>> ; Store in1 to the
allocas<br>
>> call void
@llvm.masked.scatter.v2i32(<2 x
i32> %in1.v, <2 x i32*>
%tmp,<br>
>> i32 1, <2 x i1>
<i1 true, i1 true>);<br>
>> ; Read in1 from the
allocas<br>
>> %tmp.v.0 = call <2 x
i32>
@llvm.masked.gather.v2i32(<2 x
i32*> %tmp, i32<br>
>> 1, <2 x i1> <i1
true, i1 true>, <2 x i32>
undef) #1<br>
>> ; Store in2 to the
allocas<br>
>> call void
@llvm.masked.scatter.v2i32(<2 x
i32> %in2.v, <2 x i32*>
%tmp,<br>
>> i32 1, <2 x i1>
<i1 true, i1 true>);<br>
>> ; Read in2 from the
allocas<br>
>> %tmp.v.1 = call <2 x
i32>
@llvm.masked.gather.v2i32(<2 x
i32*> %tmp, i32<br>
>> 1, <2 x i1> <i1
true, i1 true>, <2 x i32>
undef) #1<br>
>> ; Store in2 to out for
good measure<br>
>> %tmp.v.1.0 =
extractelement <2 x i32>
%tmp.v.1, i32 0<br>
>> %tmp.v.1.1 =
extractelement <2 x i32>
%tmp.v.1, i32 1<br>
>> store i32 %tmp.v.1.0,
i32* %out<br>
>> %out.1 = getelementptr
i32, i32* %out, i32 1<br>
>> store i32 %tmp.v.1.1,
i32* %out.1<br>
>> ret void<br>
>> }<br>
><br>
><br>
> It uses a masked scatter
operation to store a value to the
two allocas and<br>
> then uses a masked gather
operation to read that same value.
This is done<br>
> twice in a row, with two
different values. If I run this code
through the<br>
> GVN pass, the second gather
(%tmp.v.1) will be deemed to be the
same as the<br>
> first gather (%tmp.v.0) and it
will be removed. After some
debugging, I<br>
> realized that this is happening
because the Memory Dependence
Analysis<br>
> returns %tmp.v.0 as the Def
dependency for %tmp.v.1, even though
the scatter<br>
> call in between changes the
value stored at %tmp. This, in turn,
is<br>
> happening because the alias
analysis is returning NoModRef for
the %tmp.v.1<br>
> and second scatter callsites.
The resulting IR produces the wrong
result:<br>
><br>
>> define spir_kernel void
@test(<2 x i32*> %in1, <2 x
i32*> %in2, i32* %out)<br>
>> {<br>
>> entry:<br>
>> %tmp.0 = alloca i32<br>
>> %tmp.1 = alloca i32<br>
>> %tmp.i = insertelement
<2 x i32*> undef, i32* %tmp.0,
i32 0<br>
>> %tmp = insertelement
<2 x i32*> %tmp.i, i32*
%tmp.1, i32 1<br>
>> %in1.v = call <2 x
i32>
@llvm.masked.gather.v2i32(<2 x
i32*> %in1, i32<br>
>> 1, <2 x i1> <i1
true, i1 true>, <2 x i32>
undef) #1<br>
>> %in2.v = call <2 x
i32>
@llvm.masked.gather.v2i32(<2 x
i32*> %in2, i32<br>
>> 1, <2 x i1> <i1
true, i1 true>, <2 x i32>
undef) #1<br>
>> call void
@llvm.masked.scatter.v2i32(<2 x
i32> %in1.v, <2 x i32*>
%tmp,<br>
>> i32 1, <2 x i1>
<i1 true, i1 true>)<br>
>> %tmp.v.0 = call <2 x
i32>
@llvm.masked.gather.v2i32(<2 x
i32*> %tmp, i32<br>
>> 1, <2 x i1> <i1
true, i1 true>, <2 x i32>
undef) #1<br>
>> call void
@llvm.masked.scatter.v2i32(<2 x
i32> %in2.v, <2 x i32*>
%tmp,<br>
>> i32 1, <2 x i1>
<i1 true, i1 true>)<br>
>> ; The call to
masked.gather is gone and now we are
using the old value<br>
>> (%tmp.v.0)<br>
>> %tmp.v.1.0 =
extractelement <2 x i32>
%tmp.v.0, i32 0<br>
>> %tmp.v.1.1 =
extractelement <2 x i32>
%tmp.v.0, i32 1<br>
>> store i32 %tmp.v.1.0,
i32* %out<br>
>> %out.1 = getelementptr
i32, i32* %out, i32 1<br>
>> store i32 %tmp.v.1.1,
i32* %out.1<br>
>> ret void<br>
>> }<br>
><br>
><br>
> The old value read from %tmp is
used, instead of the new one. I
tested this<br>
> code using `opt -gvn`, with
LLVM 3.8.1. I also tried tip
(84cb7f4) with the<br>
> same result.<br>
><br>
> I should mention that if I
replace the second scatter with
stores, the issue<br>
> persists. The only way to avoid
it is to replace all scatter/gather<br>
> intrinsics with equivalent sets
of store/load, in which case the
MemDep<br>
> returns the correct
dependencies and the GVN pass will
not remove the second<br>
> set of loads.<br>
><br>
> So, my question is, is this a
bug or am I doing something that I
shouldn't<br>
> be in the IR? And if it is a
bug, is it the AA analyses that
return the<br>
> wrong result (I presume so) or
should GVN handle such cases
differently?<br>
><br>
> Best regards,<br>
><br>
> Chris<br>
><br>
</div>
</div>
> ______________________________<wbr>_________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-dev</a><br>
><br>
<span><font color="#888888"><br>
<br>
<br>
--<br>
Davide<br>
<br>
"There are no solved problems; there
are only problems that are more<br>
or less solved" -- Henri Poincare<br>
</font></span></blockquote>
</div>
<br>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</blockquote>
<br>
</div></blockquote></div></div></div></div></div>
</blockquote></div><br></div>
</blockquote><br><br><br></div></div><span><font color="#888888">-- <br><div><span></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span></span><br></div></font></span></div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div>
</blockquote><br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></div></div></div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>