[PATCH] Handle accesses to sturctures with embedded arrays and different offsets better
Nick Lewycky
nlewycky at google.com
Fri Mar 20 12:56:11 PDT 2015
On 20 March 2015 at 12:25, Daniel Berlin <dberlin at dberlin.org> wrote:
> 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).
>
Oh. :-( Yeah, we need this to work. There isn't any prohibition against
using getelementptr with negative indices to go from an element pointer to
the container pointer (eg. from int S00::*mI_2 to struct S00*). std::vector
does this, for one. Sorry.
There may be ways to achieve this optimization anyways. If "struct S00* a"
means that it needs to be a valid struct S00* pointer at the language
level, we can annotate it somehow ala. TBAA. Also, we can try to look at
the alignment and based on that deduce that the incoming pointer can't have
been arranged to conflict (ie., pointers have 16-byte alignment, the
members are 1-byte apart).
Nick
> 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/e22c86d6/attachment.html>
More information about the llvm-commits
mailing list