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

[Dan Gohman]

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/20140415/a55f6b3f/attachment.html>