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

[Daniel Berlin]

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.





> %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/9482b8e5/attachment.html>