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

[Nick Lewycky]

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/6407e0d0/attachment.html>