[PATCH] Handle accesses to sturctures with embedded arrays and different offsets better

[Daniel Berlin]

If you want this to work  we have serious problems, since the same can be
said for simple structs as well:

struct S00 {
    char mC_0;
    short mS_1;
    int mI_2;
    int mI_3;
};
void test_s00b(struct S00* a, struct S00* b)
{
  a->mI_2=1;
  b->mI_3=2;

  // c++11:   NoAlias
  // llvm:    NoAlias
  // future:  NoAlias
}

You can play the same trick here, and change offset of b so that mi_2 and
mi_3 are in the same place, and both are ints.


(Jeroen produced these results saying they are NoAlias).





On Fri, Mar 20, 2015 at 12:09 PM, Nick Lewycky <nlewycky at google.com> wrote:

> On 20 March 2015 at 11:58, Daniel Berlin <dberlin at dberlin.org> wrote:
>
>>
>>
>> On Fri, Mar 20, 2015 at 11:41 AM, Nick Lewycky <nlewycky at google.com>
>> wrote:
>>
>>> ================
>>> Comment at: lib/Analysis/BasicAliasAnalysis.cpp:979
>>> @@ -978,1 +978,3 @@
>>>
>>> +/// isActualObject -  Return true if V represents an actual object
>>> where we can
>>> +/// reason about the offsets directly, like an argument, alloca, or
>>> global
>>> ----------------
>>> I really don't understand this comment, either from "actual" or "reason
>>> about the offsets directly".
>>>
>>> Looking at the uses, I don't see any reason not to use
>>> llvm::isIdentifiedObject. I think what you need to know is that [V,
>>> V+Offset] does not overlap any other object. isIdentifiedObject should be
>>> sufficient for that.
>>>
>>
>> It would be, except isIdentifiedObject does not consider Arguments to be
>> identified unless they are noalias or byval.
>>
>> We don't care.
>> The only case noalias could matter is if they are in a union, and as
>> we've been through on this thread, we don't consider passing addresses of
>> union members to save you.
>>
>>
>>
>>>
>>> ================
>>> Comment at: lib/Analysis/BasicAliasAnalysis.cpp:983
>>> @@ +982,3 @@
>>> +static bool isActualObject(const Value *V) {
>>> +  if (isa<GlobalValue>(V) || isa<Argument>(V) || isa<AllocaInst>(V))
>>> +    return true;
>>> ----------------
>>> What about GlobalAlias? GlobalAlias isa GlobalValue.
>>>
>> Agreed.
>>
>>
>>>
>>> ================
>>> Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1063
>>> @@ -1054,1 +1062,3 @@
>>> +        // If they have the same offsets, and the underlying object is
>>> noalias,
>>> +        // they must not alias
>>>          if (GEP1BaseOffset == GEP2BaseOffset &&
>>> ----------------
>>> Full stop after "alias".
>>>
>> Fixed
>>
>>
>>>
>>> ================
>>> Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1069
>>> @@ +1068,3 @@
>>> +      } else {
>>> +        // If these are both  arguments, global variable, or alloca,
>>> and the
>>> +        // [offset, offset+size] ranges don't overlap, they can't alias
>>> ----------------
>>> Extra space between "both" and "arguments".
>>>
>> Fixed
>>
>>>
>>> ================
>>> Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1069
>>> @@ +1068,3 @@
>>> +      } else {
>>> +        // If these are both  arguments, global variable, or alloca,
>>> and the
>>> +        // [offset, offset+size] ranges don't overlap, they can't alias
>>> ----------------
>>> nlewycky wrote:
>>> > Extra space between "both" and "arguments".
>>> Tense agreement between "arguments" and "global variable" and "alloca".
>>>
>>
>>
>>> Fixed
>>>
>>
>>
>>> ================
>>> Comment at: lib/Analysis/BasicAliasAnalysis.cpp:1070
>>> @@ +1069,3 @@
>>> +        // If these are both  arguments, global variable, or alloca,
>>> and the
>>> +        // [offset, offset+size] ranges don't overlap, they can't alias
>>> +        if (isActualObject(UnderlyingV1) &&
>>> isActualObject(UnderlyingV2) &&
>>> ----------------
>>> Full stop after "alias".
>>>
>> Fixed.
>>
>>
>>>
>>> ================
>>> Comment at: test/Analysis/BasicAA/gep-alias.ll:235
>>> @@ +234,3 @@
>>> +
>>> +; We should be able to determine the second store and the load do not
>>> conflict,
>>> +; but the first store and the load are must alias
>>> ----------------
>>> But they can, I just need to call @test13 with %a = %b + 13 .
>>
>>
>>
>> I'm not sure i see how this works.
>>
>> Did you mean to check whether arguments are noalias?
>>>
>>> No.
>> The original testcase is:
>> // struct
>> struct S00 {
>>     char mC_0;
>>     short mS_1;
>>     int mI_2;
>>     int mI_3;
>> };
>>
>> // array member
>> // ------------
>> struct S03 {
>>   short mS_0;
>>   struct S00 mS00_1[4];
>>   short mS_2;
>> };
>>
>> int test_s03c(struct S03* a, struct S03* b)
>> {
>>   a->mS00_1[0].mI_2=1;
>>   b->mS00_1[1].mI_2=2;
>>   return   a->mS00_1[0].mI_2;
>> }
>>
>> I don't see how you an make these conflict, no matter what you pass in,
>> because the [1] prevents it.
>>
>
> All I need to do is arrange that &a->mS00_1[0].mI_2 == &b->mS00_1[1].mI_2.
>
> void test() {
>   int mI_2;
>   char *a = (char*)&mI_2, *b = (char*)&mI_2;
>   a -= offsetof(S03, mS00_1[0].mI_2);
>   b -= offsetof(S03, mS00_1[1].mI_2);
>   test_s03c((S03*)a, (S03*)b);
> }
>
> Nick
>
> %6 is %b+17 (aka gep %5, i32 0, i32 1, i32 2)
>>> %10 loads %a+5 (aka gep %a, i32 0, i32 1, i32 0, i32 2)
>>>
>>>
>>
>>> [I assume one byte of padding in %struct.S00 after the i16.]
>>>
>>> ================
>>> Comment at: test/Analysis/BasicAA/gep-alias.ll:236
>>> @@ +235,3 @@
>>> +; We should be able to determine the second store and the load do not
>>> conflict,
>>> +; but the first store and the load are must alias
>>> +define i32 @test13(%struct.S03* %a, %struct.S03* %b) {
>>> ----------------
>>> Full stop after "alias".
>>>
>>> http://reviews.llvm.org/D8487
>>>
>>> EMAIL PREFERENCES
>>>   http://reviews.llvm.org/settings/panel/emailpreferences/
>>>
>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150320/f9678fc2/attachment.html>