Re: [PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods

[Alina Sbirlea via cfe-commits]

Following up on this, AFAICT this is working as intended on the LLVM side.

The different decision is made in GlobalsAA + MemoryDependencyAnalysis.
IIUC, the logic there is along the lines of: if I have a global G that's
"internal" ("static" in C), and an intrinsic method M, then M cannot read
from or write to G. In the LLVM IR for the test, @deallocCalled is an
internal global, @llvm.objc.autoreleasePoolPop is an intrinsic, hence
@llvm.objc.autoreleasePoolPop cannot read or write @deallocCalled.

Please note however that there are a couple of things that I found and
intend to fix in GlobalsAA, but they do not seem to affect this case.

The one problem in particular that I thought was relevant here is that the
"MayReadAnyGlobal" is not set on a path in
GlobalsAAResult::AnalyzeCallGraph, but should apply when the function is
not an intrinsic (judging by the other code paths). If
@llvm.objc.autoreleasePoolPop was not an intrinsic, then with the fix in
D69690  would resolve this miscompile.
Perhaps we should not rely on the assumption that intrinsics are restricted
to this behavior in GlobalsAA?

Best,
Alina


On Thu, Oct 17, 2019 at 3:00 PM Alina Sbirlea 
wrote:

> I only got a chance to look more into this now. I can reproduce it with
> re-inserting the "static".
>
> The miscompile is not related to MemorySSA, i.e. disabling all uses of
> MemorySSA doesn't help.
> It appears to be a GVN bug, but I haven't tracked it further than this.
>
> To repro, see attached .ll files. The only difference is the global
> variable being static or not, which in IR translates to internal or
> dso_local.
> A simple `opt -O3 AssociatedObject_O0_*.ll` shows the difference you
> mention.
>
> Reducing the pipeline, I believe the result after the following command is
> incorrect.
> `opt -globals-aa -gvn AssociatedObject_O0_*.ll`
> I'll need to dig deeper to confirm and track down the bug inside the pass.
>
>
> Thanks,
> Alina
>
>
> On Mon, Sep 30, 2019 at 4:30 AM David Chisnall <
> david.chisn...@cl.cam.ac.uk> wrote:
>
>> Hi,
>>
>> Yes, I believe it does still reproduce (at least, with the most recent
>> build that I tried).  We worked around the clang bug to make the test
>> pass:
>>
>>
>> https://github.com/gnustep/libobjc2/commit/eab35fce379eb6ab1423dbb6d7908cb782f13458#diff-7f1eea7fdb5c7c3bf21f08d1cfe98a19
>>
>> Reintroducing the 'static' on deallocCalled reduces the test case to
>> unconditionally failing the assert immediately after the pop, and DCEs
>> the rest of the code.
>>
>> David
>>
>> On 11/09/2019 01:17, Alina Sbirlea wrote:
>> > Hi David,
>> >
>> > Does this still reproduce?
>> > I'm seeing the load after the call to autoreleasePoolPop at ToT, but
>> > perhaps I'm not using the proper flags?
>> > I only checked out the github repo and did "clang
>> > -fobjc-runtime=gnustep-2.0 -O3 -emit-llvm -S
>> > libobjc2/Test/AssociatedObject.m" with a ToT clang.
>> >
>> > Thanks,
>> > Alina
>> >
>> >
>> > On Sun, Feb 24, 2019 at 9:56 AM Pete Cooper via llvm-commits
>> > mailto:llvm-comm...@lists.llvm.org>>
>> wrote:
>> >
>> > Hey David
>> >
>> > Thanks for letting me know, and analysing it this far!
>> >
>> > I also can't see anything wrong with the intrinsic.  Its just
>> > defined as:
>> >
>> > def int_objc_autoreleasePoolPop : Intrinsic<[],
>> > [llvm_ptr_ty]>;
>> >
>> >
>> > which (I believe) means it has unmodelled side effects so it should
>> > have been fine for your example.
>> >
>> > I'll try build the same file you did and see if I can reproduce.
>> >
>> > Cheers,
>> > Pete
>> >
>> >> On Feb 24, 2019, at 7:48 AM, David Chisnall via Phabricator
>> >> mailto:revi...@reviews.llvm.org>>
>> wrote:
>> >>
>> >> theraven added a comment.
>> >> Herald added a project: LLVM.
>> >>
>> >> After some bisection, it appears that this is the revision that
>> >> introduced the regression in the GNUstep Objective-C runtime test
>> >> suite that I reported on the list a few weeks ago.  In this is the
>> >> test (compiled with `-fobjc-runtime=gnustep-2.0 -O3` and an ELF
>> >> triple):
>> >>
>> >>
>> https://github.com/gnustep/libobjc2/blob/master/Test/AssociatedObject.m
>> >>
>> >> After this change, Early CSE w/ MemorySSA is determining that the
>> >> second load of `deallocCalled` is redundant.  The code goes from:
>> >>
>> >>%7 = load i1, i1* @deallocCalled, align 1
>> >>br i1 %7, label %8, label %9
>> >>
>> >>  ; :8:  ; preds = %0
>> >>call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x
>> >> i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds
>> >> ([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 26, i8*
>> >> getelementptr inbounds ([15 x i8], [15 x i8]* @.str.1, i64 0, i64
>> >> 0)) #5
>> >>unreachable
>> >>
>> >>  ; :9:  ; preds = %0
>> >>   

Re: [PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods

[David Chisnall via cfe-commits]

Hi,

I think the assumption that intrinsics can't read any global is, indeed, 
the problem here.  `@llvm.objc.autoreleasePoolPop` may call any number 
of `-dealloc` methods with arbitrary side effects (though if they cause 
resurrection then that's undefined behaviour), so there should be no 
assumptions about the memory that it will read and write.


This check looks wrong, the documentation in Intrinsics.td states:

> By default, intrinsics are assumed to have side
> effects, so this property is only necessary if you have defined one of
> the memory properties listed above.

Looking at the other intrinsics, the ones that don't touch memory 
(mostly?) seem to have the IntrNoMem attribute set.  I think that should 
GlobalsAA should probably check that intrinsics either have that 
attribute set or one of the others that restricts the memory it can 
touch (e.g. IntrArgMemOnly).


David

On 31/10/2019 23:55, Alina Sbirlea wrote:

Following up on this, AFAICT this is working as intended on the LLVM side.

The different decision is made in GlobalsAA + MemoryDependencyAnalysis.
IIUC, the logic there is along the lines of: if I have a global G that's 
"internal" ("static" in C), and an intrinsic method M, then M cannot 
read from or write to G. In the LLVM IR for the test, @deallocCalled is 
an internal global, @llvm.objc.autoreleasePoolPop is an intrinsic, hence 
@llvm.objc.autoreleasePoolPop cannot read or write @deallocCalled.


Please note however that there are a couple of things that I found and 
intend to fix in GlobalsAA, but they do not seem to affect this case.


The one problem in particular that I thought was relevant here is that 
the "MayReadAnyGlobal" is not set on a path in 
GlobalsAAResult::AnalyzeCallGraph, but should apply when the function is 
not an intrinsic (judging by the other code paths). If 
@llvm.objc.autoreleasePoolPop was not an intrinsic, then with the fix in 
D69690  would resolve this miscompile.
Perhaps we should not rely on the assumption that intrinsics are 
restricted to this behavior in GlobalsAA?


Best,
Alina


On Thu, Oct 17, 2019 at 3:00 PM Alina Sbirlea > wrote:


I only got a chance to look more into this now. I can reproduce it
with re-inserting the "static".

The miscompile is not related to MemorySSA, i.e. disabling all uses
of MemorySSA doesn't help.
It appears to be a GVN bug, but I haven't tracked it further than this.

To repro, see attached .ll files. The only difference is the global
variable being static or not, which in IR translates to internal or
dso_local.
A simple `opt -O3 AssociatedObject_O0_*.ll` shows the difference you
mention.

Reducing the pipeline, I believe the result after the following
command is incorrect.
`opt -globals-aa -gvn AssociatedObject_O0_*.ll`
I'll need to dig deeper to confirm and track down the bug inside the
pass.


Thanks,
Alina


On Mon, Sep 30, 2019 at 4:30 AM David Chisnall
mailto:david.chisn...@cl.cam.ac.uk>>
wrote:

Hi,

Yes, I believe it does still reproduce (at least, with the most
recent
build that I tried).  We worked around the clang bug to make the
test pass:


https://github.com/gnustep/libobjc2/commit/eab35fce379eb6ab1423dbb6d7908cb782f13458#diff-7f1eea7fdb5c7c3bf21f08d1cfe98a19

Reintroducing the 'static' on deallocCalled reduces the test
case to
unconditionally failing the assert immediately after the pop,
and DCEs
the rest of the code.

David

On 11/09/2019 01:17, Alina Sbirlea wrote:
 > Hi David,
 >
 > Does this still reproduce?
 > I'm seeing the load after the call to autoreleasePoolPop at
ToT, but
 > perhaps I'm not using the proper flags?
 > I only checked out the github repo and did "clang
 > -fobjc-runtime=gnustep-2.0 -O3 -emit-llvm -S
 > libobjc2/Test/AssociatedObject.m" with a ToT clang.
 >
 > Thanks,
 > Alina
 >
 >
 > On Sun, Feb 24, 2019 at 9:56 AM Pete Cooper via llvm-commits
 > mailto:llvm-comm...@lists.llvm.org>
>> wrote:
 >
 >     Hey David
 >
 >     Thanks for letting me know, and analysing it this far!
 >
 >     I also can't see anything wrong with the intrinsic.  Its just
 >     defined as:
 >
 >         def int_objc_autoreleasePoolPop             :
Intrinsic<[],
 >         [llvm_ptr_ty]>;
 >
 >
 >     which (I believe) means it has unmodelled side effects so
it should
 >     have been fine for your example.
 >
 >     I'll try build the same file you did and see if I can
reproduce.
 >

Re: [PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods

[Alina Sbirlea via cfe-commits]

I only got a chance to look more into this now. I can reproduce it with
re-inserting the "static".

The miscompile is not related to MemorySSA, i.e. disabling all uses of
MemorySSA doesn't help.
It appears to be a GVN bug, but I haven't tracked it further than this.

To repro, see attached .ll files. The only difference is the global
variable being static or not, which in IR translates to internal or
dso_local.
A simple `opt -O3 AssociatedObject_O0_*.ll` shows the difference you
mention.

Reducing the pipeline, I believe the result after the following command is
incorrect.
`opt -globals-aa -gvn AssociatedObject_O0_*.ll`
I'll need to dig deeper to confirm and track down the bug inside the pass.


Thanks,
Alina


On Mon, Sep 30, 2019 at 4:30 AM David Chisnall 
wrote:

> Hi,
>
> Yes, I believe it does still reproduce (at least, with the most recent
> build that I tried).  We worked around the clang bug to make the test pass:
>
>
> https://github.com/gnustep/libobjc2/commit/eab35fce379eb6ab1423dbb6d7908cb782f13458#diff-7f1eea7fdb5c7c3bf21f08d1cfe98a19
>
> Reintroducing the 'static' on deallocCalled reduces the test case to
> unconditionally failing the assert immediately after the pop, and DCEs
> the rest of the code.
>
> David
>
> On 11/09/2019 01:17, Alina Sbirlea wrote:
> > Hi David,
> >
> > Does this still reproduce?
> > I'm seeing the load after the call to autoreleasePoolPop at ToT, but
> > perhaps I'm not using the proper flags?
> > I only checked out the github repo and did "clang
> > -fobjc-runtime=gnustep-2.0 -O3 -emit-llvm -S
> > libobjc2/Test/AssociatedObject.m" with a ToT clang.
> >
> > Thanks,
> > Alina
> >
> >
> > On Sun, Feb 24, 2019 at 9:56 AM Pete Cooper via llvm-commits
> > mailto:llvm-comm...@lists.llvm.org>>
> wrote:
> >
> > Hey David
> >
> > Thanks for letting me know, and analysing it this far!
> >
> > I also can't see anything wrong with the intrinsic.  Its just
> > defined as:
> >
> > def int_objc_autoreleasePoolPop : Intrinsic<[],
> > [llvm_ptr_ty]>;
> >
> >
> > which (I believe) means it has unmodelled side effects so it should
> > have been fine for your example.
> >
> > I'll try build the same file you did and see if I can reproduce.
> >
> > Cheers,
> > Pete
> >
> >> On Feb 24, 2019, at 7:48 AM, David Chisnall via Phabricator
> >> mailto:revi...@reviews.llvm.org>> wrote:
> >>
> >> theraven added a comment.
> >> Herald added a project: LLVM.
> >>
> >> After some bisection, it appears that this is the revision that
> >> introduced the regression in the GNUstep Objective-C runtime test
> >> suite that I reported on the list a few weeks ago.  In this is the
> >> test (compiled with `-fobjc-runtime=gnustep-2.0 -O3` and an ELF
> >> triple):
> >>
> >>
> https://github.com/gnustep/libobjc2/blob/master/Test/AssociatedObject.m
> >>
> >> After this change, Early CSE w/ MemorySSA is determining that the
> >> second load of `deallocCalled` is redundant.  The code goes from:
> >>
> >>%7 = load i1, i1* @deallocCalled, align 1
> >>br i1 %7, label %8, label %9
> >>
> >>  ; :8:  ; preds = %0
> >>call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x
> >> i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds
> >> ([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 26, i8*
> >> getelementptr inbounds ([15 x i8], [15 x i8]* @.str.1, i64 0, i64
> >> 0)) #5
> >>unreachable
> >>
> >>  ; :9:  ; preds = %0
> >>call void @llvm.objc.autoreleasePoolPop(i8* %1)
> >>%10 = load i1, i1* @deallocCalled, align 1
> >>br i1 %10, label %12, label %11
> >>
> >>  ; :11: ; preds = %9
> >>call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x
> >> i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds
> >> ([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 29, i8*
> >> getelementptr inbounds ([14 x i8], [14 x i8]* @.str.2, i64 0, i64
> >> 0)) #5
> >>unreachable
> >>
> >> to:
> >>
> >>%7 = load i1, i1* @deallocCalled, align 1
> >>br i1 %7, label %8, label %9
> >>
> >>  ; :8:  ; preds = %0
> >>call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x
> >> i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds
> >> ([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 26, i8*
> >> getelementptr inbounds ([15 x i8], [15 x i8]* @.str.1, i64 0, i64
> >> 0)) #5
> >>unreachable
> >>
> >>  ; :9:  ; preds = %0
> >>call void @llvm.objc.autoreleasePoolPop(i8* %1)
> >>br i1 %7, label %11, label %10
> >>
> >>  ; :10: ; preds = %9
> >>call void @__assert(i8* getelementptr inbounds ([5

Re: [PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods

[David Chisnall via cfe-commits]

Hi,

Yes, I believe it does still reproduce (at least, with the most recent 
build that I tried).  We worked around the clang bug to make the test pass:


https://github.com/gnustep/libobjc2/commit/eab35fce379eb6ab1423dbb6d7908cb782f13458#diff-7f1eea7fdb5c7c3bf21f08d1cfe98a19

Reintroducing the 'static' on deallocCalled reduces the test case to 
unconditionally failing the assert immediately after the pop, and DCEs 
the rest of the code.


David

On 11/09/2019 01:17, Alina Sbirlea wrote:

Hi David,

Does this still reproduce?
I'm seeing the load after the call to autoreleasePoolPop at ToT, but 
perhaps I'm not using the proper flags?
I only checked out the github repo and did "clang 
-fobjc-runtime=gnustep-2.0 -O3 -emit-llvm -S 
libobjc2/Test/AssociatedObject.m" with a ToT clang.


Thanks,
Alina


On Sun, Feb 24, 2019 at 9:56 AM Pete Cooper via llvm-commits 
mailto:llvm-comm...@lists.llvm.org>> wrote:


Hey David

Thanks for letting me know, and analysing it this far!

I also can't see anything wrong with the intrinsic.  Its just
defined as:

def int_objc_autoreleasePoolPop             : Intrinsic<[],
[llvm_ptr_ty]>;


which (I believe) means it has unmodelled side effects so it should
have been fine for your example.

I'll try build the same file you did and see if I can reproduce.

Cheers,
Pete


On Feb 24, 2019, at 7:48 AM, David Chisnall via Phabricator
mailto:revi...@reviews.llvm.org>> wrote:

theraven added a comment.
Herald added a project: LLVM.

After some bisection, it appears that this is the revision that
introduced the regression in the GNUstep Objective-C runtime test
suite that I reported on the list a few weeks ago.  In this is the
test (compiled with `-fobjc-runtime=gnustep-2.0 -O3` and an ELF
triple):

https://github.com/gnustep/libobjc2/blob/master/Test/AssociatedObject.m

After this change, Early CSE w/ MemorySSA is determining that the
second load of `deallocCalled` is redundant.  The code goes from:

   %7 = load i1, i1* @deallocCalled, align 1
   br i1 %7, label %8, label %9

 ; :8:  ; preds = %0
   call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x
i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds
([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 26, i8*
getelementptr inbounds ([15 x i8], [15 x i8]* @.str.1, i64 0, i64
0)) #5
   unreachable

 ; :9:  ; preds = %0
   call void @llvm.objc.autoreleasePoolPop(i8* %1)
   %10 = load i1, i1* @deallocCalled, align 1
   br i1 %10, label %12, label %11

 ; :11: ; preds = %9
   call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x
i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds
([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 29, i8*
getelementptr inbounds ([14 x i8], [14 x i8]* @.str.2, i64 0, i64
0)) #5
   unreachable

to:

   %7 = load i1, i1* @deallocCalled, align 1
   br i1 %7, label %8, label %9

 ; :8:  ; preds = %0
   call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x
i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds
([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 26, i8*
getelementptr inbounds ([15 x i8], [15 x i8]* @.str.1, i64 0, i64
0)) #5
   unreachable

 ; :9:  ; preds = %0
   call void @llvm.objc.autoreleasePoolPop(i8* %1)
   br i1 %7, label %11, label %10

 ; :10: ; preds = %9
   call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x
i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds
([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 29, i8*
getelementptr inbounds ([14 x i8], [14 x i8]* @.str.2, i64 0, i64
0)) #5
   unreachable

Later optimisations then determine that, because the assert does
not return, the only possible value for %7 is false and cause the
second assert to fire unconditionally.

It appears that we are not correctly modelling the side effects of
the `llvm.objc.autoreleasePoolPop` intrinsic, but it's not
entirely clear why not.  The same test compiled for the macos
runtime does not appear to exhibit the same behaviour.  The
previous revision, where we emitted a call to
`objc_autoreleasePoolPop` and not the intrinsic worked correctly,
but with this change the optimisers are assuming that no globals
can be modified across an autorelease pool pop operation (at
least, in some situations).

Looking at the definition of the intrinsic, I don't see anything
wrong, so I still suspect that there is a MemorySSA bug that this
has uncovered, rather than anything wrong in this series of
commits.  Any sugge

Re: [PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods

[Alina Sbirlea via cfe-commits]

Hi David,

Does this still reproduce?
I'm seeing the load after the call to autoreleasePoolPop at ToT, but
perhaps I'm not using the proper flags?
I only checked out the github repo and did "clang
-fobjc-runtime=gnustep-2.0 -O3 -emit-llvm -S
libobjc2/Test/AssociatedObject.m" with a ToT clang.

Thanks,
Alina


On Sun, Feb 24, 2019 at 9:56 AM Pete Cooper via llvm-commits <
llvm-comm...@lists.llvm.org> wrote:

> Hey David
>
> Thanks for letting me know, and analysing it this far!
>
> I also can't see anything wrong with the intrinsic.  Its just defined as:
>
> def int_objc_autoreleasePoolPop : Intrinsic<[], [llvm_ptr_ty]>;
>
>
> which (I believe) means it has unmodelled side effects so it should have
> been fine for your example.
>
> I'll try build the same file you did and see if I can reproduce.
>
> Cheers,
> Pete
>
> On Feb 24, 2019, at 7:48 AM, David Chisnall via Phabricator <
> revi...@reviews.llvm.org> wrote:
>
> theraven added a comment.
> Herald added a project: LLVM.
>
> After some bisection, it appears that this is the revision that introduced
> the regression in the GNUstep Objective-C runtime test suite that I
> reported on the list a few weeks ago.  In this is the test (compiled with
> `-fobjc-runtime=gnustep-2.0 -O3` and an ELF triple):
>
> https://github.com/gnustep/libobjc2/blob/master/Test/AssociatedObject.m
>
> After this change, Early CSE w/ MemorySSA is determining that the second
> load of `deallocCalled` is redundant.  The code goes from:
>
>%7 = load i1, i1* @deallocCalled, align 1
>br i1 %7, label %8, label %9
>
>  ; :8:  ; preds = %0
>call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]*
> @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x
> i8]* @.str, i64 0, i64 0), i32 26, i8* getelementptr inbounds ([15 x i8],
> [15 x i8]* @.str.1, i64 0, i64 0)) #5
>unreachable
>
>  ; :9:  ; preds = %0
>call void @llvm.objc.autoreleasePoolPop(i8* %1)
>%10 = load i1, i1* @deallocCalled, align 1
>br i1 %10, label %12, label %11
>
>  ; :11: ; preds = %9
>call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]*
> @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x
> i8]* @.str, i64 0, i64 0), i32 29, i8* getelementptr inbounds ([14 x i8],
> [14 x i8]* @.str.2, i64 0, i64 0)) #5
>unreachable
>
> to:
>
>%7 = load i1, i1* @deallocCalled, align 1
>br i1 %7, label %8, label %9
>
>  ; :8:  ; preds = %0
>call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]*
> @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x
> i8]* @.str, i64 0, i64 0), i32 26, i8* getelementptr inbounds ([15 x i8],
> [15 x i8]* @.str.1, i64 0, i64 0)) #5
>unreachable
>
>  ; :9:  ; preds = %0
>call void @llvm.objc.autoreleasePoolPop(i8* %1)
>br i1 %7, label %11, label %10
>
>  ; :10: ; preds = %9
>call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]*
> @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x
> i8]* @.str, i64 0, i64 0), i32 29, i8* getelementptr inbounds ([14 x i8],
> [14 x i8]* @.str.2, i64 0, i64 0)) #5
>unreachable
>
> Later optimisations then determine that, because the assert does not
> return, the only possible value for %7 is false and cause the second assert
> to fire unconditionally.
>
> It appears that we are not correctly modelling the side effects of the
> `llvm.objc.autoreleasePoolPop` intrinsic, but it's not entirely clear why
> not.  The same test compiled for the macos runtime does not appear to
> exhibit the same behaviour.  The previous revision, where we emitted a call
> to `objc_autoreleasePoolPop` and not the intrinsic worked correctly, but
> with this change the optimisers are assuming that no globals can be
> modified across an autorelease pool pop operation (at least, in some
> situations).
>
> Looking at the definition of the intrinsic, I don't see anything wrong, so
> I still suspect that there is a MemorySSA bug that this has uncovered,
> rather than anything wrong in this series of commits.  Any suggestions as
> to where to look would be appreciated.
>
>
> Repository:
>  rL LLVM
>
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D55802/new/
>
> https://reviews.llvm.org/D55802
>
>
>
>
> ___
> llvm-commits mailing list
> llvm-comm...@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods

[Pete Cooper via cfe-commits]

Hey David

Thanks for letting me know, and analysing it this far!

I also can't see anything wrong with the intrinsic.  Its just defined as:

def int_objc_autoreleasePoolPop : Intrinsic<[], [llvm_ptr_ty]>;

which (I believe) means it has unmodelled side effects so it should have been 
fine for your example.

I'll try build the same file you did and see if I can reproduce.

Cheers,
Pete

> On Feb 24, 2019, at 7:48 AM, David Chisnall via Phabricator 
>  wrote:
> 
> theraven added a comment.
> Herald added a project: LLVM.
> 
> After some bisection, it appears that this is the revision that introduced 
> the regression in the GNUstep Objective-C runtime test suite that I reported 
> on the list a few weeks ago.  In this is the test (compiled with 
> `-fobjc-runtime=gnustep-2.0 -O3` and an ELF triple):
> 
> https://github.com/gnustep/libobjc2/blob/master/Test/AssociatedObject.m
> 
> After this change, Early CSE w/ MemorySSA is determining that the second load 
> of `deallocCalled` is redundant.  The code goes from:
> 
>%7 = load i1, i1* @deallocCalled, align 1
>br i1 %7, label %8, label %9
> 
>  ; :8:  ; preds = %0
>call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* 
> @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x 
> i8]* @.str, i64 0, i64 0), i32 26, i8* getelementptr inbounds ([15 x i8], [15 
> x i8]* @.str.1, i64 0, i64 0)) #5
>unreachable
> 
>  ; :9:  ; preds = %0
>call void @llvm.objc.autoreleasePoolPop(i8* %1)
>%10 = load i1, i1* @deallocCalled, align 1
>br i1 %10, label %12, label %11
> 
>  ; :11: ; preds = %9
>call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* 
> @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x 
> i8]* @.str, i64 0, i64 0), i32 29, i8* getelementptr inbounds ([14 x i8], [14 
> x i8]* @.str.2, i64 0, i64 0)) #5
>unreachable
> 
> to:
> 
>%7 = load i1, i1* @deallocCalled, align 1
>br i1 %7, label %8, label %9
> 
>  ; :8:  ; preds = %0
>call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* 
> @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x 
> i8]* @.str, i64 0, i64 0), i32 26, i8* getelementptr inbounds ([15 x i8], [15 
> x i8]* @.str.1, i64 0, i64 0)) #5
>unreachable
> 
>  ; :9:  ; preds = %0
>call void @llvm.objc.autoreleasePoolPop(i8* %1)
>br i1 %7, label %11, label %10
> 
>  ; :10: ; preds = %9
>call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* 
> @__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x 
> i8]* @.str, i64 0, i64 0), i32 29, i8* getelementptr inbounds ([14 x i8], [14 
> x i8]* @.str.2, i64 0, i64 0)) #5
>unreachable
> 
> Later optimisations then determine that, because the assert does not return, 
> the only possible value for %7 is false and cause the second assert to fire 
> unconditionally.
> 
> It appears that we are not correctly modelling the side effects of the 
> `llvm.objc.autoreleasePoolPop` intrinsic, but it's not entirely clear why 
> not.  The same test compiled for the macos runtime does not appear to exhibit 
> the same behaviour.  The previous revision, where we emitted a call to 
> `objc_autoreleasePoolPop` and not the intrinsic worked correctly, but with 
> this change the optimisers are assuming that no globals can be modified 
> across an autorelease pool pop operation (at least, in some situations).
> 
> Looking at the definition of the intrinsic, I don't see anything wrong, so I 
> still suspect that there is a MemorySSA bug that this has uncovered, rather 
> than anything wrong in this series of commits.  Any suggestions as to where 
> to look would be appreciated.
> 
> 
> Repository:
>  rL LLVM
> 
> CHANGES SINCE LAST ACTION
>  https://reviews.llvm.org/D55802/new/
> 
> https://reviews.llvm.org/D55802
> 
> 
> 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods

[David Chisnall via Phabricator via cfe-commits]

theraven added a comment.
Herald added a project: LLVM.

After some bisection, it appears that this is the revision that introduced the 
regression in the GNUstep Objective-C runtime test suite that I reported on the 
list a few weeks ago.  In this is the test (compiled with 
`-fobjc-runtime=gnustep-2.0 -O3` and an ELF triple):

https://github.com/gnustep/libobjc2/blob/master/Test/AssociatedObject.m

After this change, Early CSE w/ MemorySSA is determining that the second load 
of `deallocCalled` is redundant.  The code goes from:

%7 = load i1, i1* @deallocCalled, align 1
br i1 %7, label %8, label %9
  
  ; :8:  ; preds = %0
call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* 
@__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x 
i8]* @.str, i64 0, i64 0), i32 26, i8* getelementptr inbounds ([15 x i8], [15 x 
i8]* @.str.1, i64 0, i64 0)) #5
unreachable
  
  ; :9:  ; preds = %0
call void @llvm.objc.autoreleasePoolPop(i8* %1)
%10 = load i1, i1* @deallocCalled, align 1
br i1 %10, label %12, label %11
  
  ; :11: ; preds = %9
call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* 
@__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x 
i8]* @.str, i64 0, i64 0), i32 29, i8* getelementptr inbounds ([14 x i8], [14 x 
i8]* @.str.2, i64 0, i64 0)) #5
unreachable

to:

%7 = load i1, i1* @deallocCalled, align 1
br i1 %7, label %8, label %9
  
  ; :8:  ; preds = %0
call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* 
@__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x 
i8]* @.str, i64 0, i64 0), i32 26, i8* getelementptr inbounds ([15 x i8], [15 x 
i8]* @.str.1, i64 0, i64 0)) #5
unreachable
  
  ; :9:  ; preds = %0
call void @llvm.objc.autoreleasePoolPop(i8* %1)
br i1 %7, label %11, label %10
  
  ; :10: ; preds = %9
call void @__assert(i8* getelementptr inbounds ([5 x i8], [5 x i8]* 
@__func__.main, i64 0, i64 0), i8* getelementptr inbounds ([27 x i8], [27 x 
i8]* @.str, i64 0, i64 0), i32 29, i8* getelementptr inbounds ([14 x i8], [14 x 
i8]* @.str.2, i64 0, i64 0)) #5
unreachable

Later optimisations then determine that, because the assert does not return, 
the only possible value for %7 is false and cause the second assert to fire 
unconditionally.

It appears that we are not correctly modelling the side effects of the 
`llvm.objc.autoreleasePoolPop` intrinsic, but it's not entirely clear why not.  
The same test compiled for the macos runtime does not appear to exhibit the 
same behaviour.  The previous revision, where we emitted a call to 
`objc_autoreleasePoolPop` and not the intrinsic worked correctly, but with this 
change the optimisers are assuming that no globals can be modified across an 
autorelease pool pop operation (at least, in some situations).

Looking at the definition of the intrinsic, I don't see anything wrong, so I 
still suspect that there is a MemorySSA bug that this has uncovered, rather 
than anything wrong in this series of commits.  Any suggestions as to where to 
look would be appreciated.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55802/new/

https://reviews.llvm.org/D55802



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods

[Pete Cooper via Phabricator via cfe-commits]

pete added a comment.

In D55802#1334008 , @rjmccall wrote:

> You're making intrinsics with `weak_external` linkage?  I feel like that's 
> going to be unnecessarily awkward in the future, but okay.


Yeah... i was a little surprised that was possible, but seems to work just fine.

Thanks for the quick review!  Will land it first thing tomorrow.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55802/new/

https://reviews.llvm.org/D55802



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

You're making intrinsics with `weak_external` linkage?  I feel like that's 
going to be unnecessarily awkward in the future, but okay.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55802/new/

https://reviews.llvm.org/D55802



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits