<div dir="ltr">Thanks for the inputs and clarifications Dan. Really appreciate it. May be as you suggested i will leave this bug as it is.<div>Thanks</div><div>Karthik</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Tue, Apr 15, 2014 at 11:12 PM, Dan Gohman <span dir="ltr"><<a href="mailto:dan433584@gmail.com" target="_blank">dan433584@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">Hello,<br><br>I added a comment to PR16236.<br><div><div class="gmail_extra"><br>Also, to clarify my remark below about pointers being bitcasted, LLVM's own type system provides no aliasing guarantees. Under LLVM type system rules, a pointer with type T* may point to any memory, regardless of whether anyone thinks there's a T in memory there. Consequently, you can't determine if something is a union or not by asking the LLVM type system. The only way that TBAA in LLVM can work is through the use of a separate and independent type system, described in metadata.<div>
<div class="h5"><br>
<br><div class="gmail_quote">On Mon, Apr 14, 2014 at 9:21 PM, Karthik Bhat <span dir="ltr"><<a href="mailto:blitz.opensource@gmail.com" target="_blank">blitz.opensource@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">Hi Dan,<div>It would be great to know your inputs on this patch and if you could clarify few doubts as mentioned below.</div><div>Thanks a lot!</div><div><br></div><div>Regards</div><div><span><font color="#888888">Karthik Bhat</font></span><div>
<div><br><div class="gmail_extra">
<br><br><div class="gmail_quote">On Wed, Apr 9, 2014 at 9:44 AM, Karthik Bhat <span dir="ltr"><<a href="mailto:blitz.opensource@gmail.com" target="_blank">blitz.opensource@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr">Thanks Dan for the review. I have a few doubts though if you could clarify the same.<br><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Wed, Apr 9, 2014 at 12:17 AM, Dan Gohman <span dir="ltr"><<a href="mailto:dan433584@gmail.com" target="_blank">dan433584@gmail.com</a>></span> wrote:<br>
</div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra">
<br><br><div class="gmail_quote">
<div>On Tue, Apr 8, 2014 at 5:04 AM, Karthik Bhat <span dir="ltr"><<a href="mailto:kv.bhat@samsung.com" target="_blank">kv.bhat@samsung.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi majnemer, sunfish, rengolin,<br>
<br>
Hi All,<br>
This patches fixes PR16236 aproblem with TypeBasedAlias Analysis. Below is my understanding please help me review this patch and if i'm thinking in the right direction.<br>
The problem in this case seems to be that in case we have an union like -<br>
union<br>
{<br>
short f0;<br>
int f1;<br>
} u, *up = &u;<br>
<br>
f0 and f1 may alias each other. But while constructing AliasSet for these in TypeBasedAlias Analysis during PathAliases we see that both the MDNodes corresponding to (f0 and f1) are not ancestor of each other and have a common root and thus conclude that they will Not alias each other. But in case of union this may not be correct f1 anf f0 may alias each other as thye operate on same underlining memory location.<br>
</blockquote><div><br></div></div><div>LLVM's approach for solving this problem is to do a BasicAA query first, and if BasicAA says MustAlias or PartialAlias, it doesn't do a TBAA query at all. This represents a kind of "best effort": the optimizer will make its best effort to discover whether the type information is unreliable, but if it doesn't discover a problem, then it assumes that the type information is reliable. As such, it's not safe in all cases, but the C standard's own definition of TBAA is not safe in all cases, and this is an attempt to implement the spirit of what the C standard desired while finding a practical balance with correctness.<br>
</div><div><br>Are you seeing a case where this approach is not sufficient?<br></div></div></div></div></blockquote><div><br></div></div><div style="font-family:arial,sans-serif;font-size:13px">Yes Dan in PR16236. <a href="http://llvm.org/bugs/show_bug.cgi?id=16236" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=16236</a> we have an example were the behavior with and without optimization is different. Ideally optimization should not change program behavior. Given the below program from the bugID-</div>
<div style="font-family:arial,sans-serif;font-size:13px"><pre style="white-space:pre-wrap;width:50em">int printf(const char *, ...);
union
{
short f0;
int f1;
} u, *up = &u;
int a;
int main ()
{
for (; a <= 0; a++)
if ((u.f0 = 1))
up->f1 = 0;
printf ("%d\n", u.f1);
return 0;
}</pre></div><div><span style="font-family:arial,sans-serif;font-size:13px">TypeBasedAlias analysis incorrectly returns NoAlias for u.f0 and up->f1 as a result the LICM transformation moves the store to u.f0 out of the loop resulting in the output of the program to be 1 instead of 0 when we compile with O1 or above. </span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">What this patch does is in tbaa we add a check if we are comparing 2 elemtents of a struct type( since we cannot differentiate b/w struct and union) say tbaa *cannot* conclude that these are NoAlias/MustAlias (in case of union they actually alias each other and in case of struct they may not. So we *cannot* strictly say </span><span style="font-family:arial,sans-serif;font-size:13px;white-space:pre-wrap">u.f0 and </span><span style="font-family:arial,sans-serif;font-size:13px;white-space:pre-wrap">up->f1</span><span style="font-family:arial,sans-serif;font-size:13px"> are NoAlias) and hence we conservatively exit TypeBasedAnalysis for this and call other aliasAnalysis.</span></div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div></div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
In this patch we check that if both correspond to StructTyID type we evaluate it conservatively so that we do not incorrectly conclude that these two types are NoAlias.<br></blockquote><div><br></div></div><div>This is not safe, since pointers can be bitcasted to pointer types with arbitrary pointees.</div>
</div></div></div></blockquote><div> </div></div><div><span style="font-family:arial,sans-serif;font-size:13px">Please correct me if my understanding is incorrect but ideally since we are just saying somethig like TBAA *cannot* conclude if these 2 pointers are Alias or not and *so proceed to next alias analysis* this should be safer and more conservative than concluding that 2 struct element are NoAlias like in current code which may be false in case of union resulting in incorrect optimizations.</span><br>
</div></div></div></div></blockquote></div></div></div></div></div></div></blockquote><br></div></div></div></div></div></div>
</blockquote></div><br></div>