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

[Karthik Bhat]

Thanks for the inputs and clarifications Dan. Really appreciate it. May be
as you suggested i will leave this bug as it is.
Thanks
Karthik


On Tue, Apr 15, 2014 at 11:12 PM, Dan Gohman <dan433584 at gmail.com> wrote:

> Hello,
>
> I added a comment to PR16236.
>
> 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.
>
>
> On Mon, Apr 14, 2014 at 9:21 PM, Karthik Bhat <blitz.opensource at gmail.com>wrote:
>
>> 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.
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140416/08f77933/attachment.html>