[PATCH] [PR16236] Be conservative when doing TypeBasedAlias Analysis of members of Struct

[Karthik Bhat]

Hi Dan,
It would be great to know your inputs on this patch and if you could
clarify few doubts as mentioned below.
Thanks a lot!

Regards
Karthik Bhat


On Wed, Apr 9, 2014 at 9:44 AM, Karthik Bhat <blitz.opensource at gmail.com>wrote:

> Thanks Dan for the review. I have a few doubts though if you could clarify
> the same.
>
>
> On Wed, Apr 9, 2014 at 12:17 AM, Dan Gohman <dan433584 at gmail.com> wrote:
>
>>
>>
>>
>> On Tue, Apr 8, 2014 at 5:04 AM, Karthik Bhat <kv.bhat at samsung.com> wrote:
>>
>>> Hi majnemer, sunfish, rengolin,
>>>
>>> Hi All,
>>> 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.
>>> The problem in this case seems to be that in case we have an union like -
>>>   union
>>>   {
>>>     short f0;
>>>     int f1;
>>>   } u, *up = &u;
>>>
>>> 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.
>>>
>>
>> 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.
>>
>> Are you seeing a case where this approach is not sufficient?
>>
>
> Yes Dan in PR16236. http://llvm.org/bugs/show_bug.cgi?id=16236 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-
>
> 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;
> }
>
> 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.
> 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 u.f0 and up->f1 are NoAlias) and hence we
> conservatively exit TypeBasedAnalysis for this and call other aliasAnalysis.
>
>
>>
>>
>>> 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.
>>>
>>
>> This is not safe, since pointers can be bitcasted to pointer types with
>> arbitrary pointees.
>>
>
> 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.
>
> Thanks
> Karthik
>
>
>>
>>
>> Dan
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140415/14d667e9/attachment.html>