<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 21 May 2014 13:40, Robert Lougher <span dir="ltr"><<a href="mailto:rob.lougher@gmail.com" target="_blank">rob.lougher@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi,<br>
<br>
I'm investigating a bug which I have so far been able to narrow down<br>
to the following small testcase:<br>
<br>
======== test.c ===========<br>
<br>
int *get_pntr(int *p) {<br>
return p;<br>
}<br>
<br>
__attribute__((noinline))<br>
void store(int *p) {<br>
int *p2 = get_pntr(p);<br>
*p2 = 10;<br>
}<br>
<br>
int test() {<br>
int i;<br>
store(&i);<br>
return i;<br>
}<br>
-----------------------------<br>
<br>
If this is compiled in two steps as follows:<br>
<br>
clang -O1 -emit-llvm test.c -c -o test.bc<br>
opt -basicaa -inline -functionattrs -gvn -S test.bc<br>
<br>
We get the following IR:<br>
<br>
--------------------------------------------------<br>
define i32* @get_pntr(i32* readnone %p) {<br>
entry:<br>
ret i32* %p<br>
}<br>
<br>
define void @store(i32* nocapture readnone %p) {<br>
entry:<br>
store i32 10, i32* %p, align 4, !tbaa !1<br>
ret void<br>
}<br>
<br>
define i32 @test() {<br>
entry:<br>
%i = alloca i32, align 4<br>
call void @store(i32* %i)<br>
ret i32 undef<br>
}<br>
--------------------------------------------------<br>
<br>
As can be seen, the load of %i in test() has been eliminated, and we<br>
have an undefined return value.<br></blockquote><div><br></div><div>As of revision 209870 we now get:</div><div><br></div><div><div>; Function Attrs: nounwind readnone uwtable</div><div>define i32* @get_pntr(i32* readnone %p) #0 {</div>
<div>entry:</div><div> ret i32* %p</div><div>}</div><div><br></div><div>; Function Attrs: noinline nounwind uwtable</div><div>define void @store(i32* nocapture %p) #1 {</div><div>entry:</div><div> store i32 10, i32* %p, align 4, !tbaa !1</div>
<div> ret void</div><div>}</div><div><br></div><div>; Function Attrs: nounwind uwtable</div><div>define i32 @test() #2 {</div><div>entry:</div><div> %i = alloca i32, align 4</div><div> call void @store(i32* %i)</div><div>
%0 = load i32* %i, align 4, !tbaa !1</div><div> ret i32 %0</div><div>}</div></div><div><br></div><div>I think that's correct, let me know if there's another bug hiding in here.</div><div><br></div><div>Nick</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Investigating this I can see that the load is removed by the GVN pass.<br>
It does this because it believes that the call to store() has no<br>
affect on %i, and as %i has just been allocated, the loaded value is<br>
undefined.<br>
<br>
The reason it believes that store() has no affect is because of<br>
store()'s parameter attributes. Argument %p doesn't escape<br>
(nocapture), and it doesn't access memory (readnone). So even though<br>
the call aliases the load, it believes it doesn't touch it (even<br>
though it clearly does).<br>
<br>
The nocapture attribute is added by the functionattrs pass (and is<br>
correct). The readnone attribute, however, is present from the first<br>
compilation step (clang -O1):<br>
<br>
--------------------------------------------------<br>
define i32* @get_pntr(i32* readnone %p) {<br>
entry:<br>
ret i32* %p<br>
}<br>
<br>
define void @store(i32* readnone %p) {<br>
entry:<br>
%call = tail call i32* @get_pntr(i32* %p)<br>
store i32 10, i32* %call, align 4, !tbaa !1<br>
ret void<br>
}<br>
<br>
define i32 @test() {<br>
entry:<br>
%i = alloca i32, align 4<br>
call void @store(i32* %i)<br>
%0 = load i32* %i, align 4, !tbaa !1<br>
ret i32 %0<br>
}<br>
--------------------------------------------------<br>
<br>
Looking at the definition of readnone in the LangRef we see:<br>
<br>
"On an argument, this attribute indicates that the function does not<br>
dereference that pointer argument, even though it may read or write<br>
the memory that the pointer points to if accessed through other<br>
pointers."<br>
<br>
So this appears to be correct. Argument %p is marked as readnone in<br>
both get_pntr() and store() as it is not read or written to. Store()<br>
does write to it via a copy of %p, but this is also OK according to<br>
the definition.<br>
<br>
So the problem appears to be within GVN. The assumption based on<br>
store()'s parameter attributes is incorrect, as the load can be<br>
affected through a different pointer.<br>
<br>
So before I try to fix GVN, my question is, am I on the right track?<br>
Specifically, is the definition in the LangRef correct?<br>
<br>
Thanks,<br>
Rob.<br>
<br>
P.S. GVN's checking of the attributes is done in<br>
AliasAnalysis::callCapturesBefore in lib/Analysis/AliasAnalysis.cpp<br>
<br>
--<br>
Robert Lougher<br>
SN Systems - Sony Computer Entertainment Group<br>
_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
</blockquote></div><br></div></div>