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

[Karthik Bhat]

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/20140409/e9605ec1/attachment.html>