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

Alina Sbirlea via cfe-commits cfe-commits at lists.llvm.org
Thu Oct 17 15:00:00 PDT 2019


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.Chisnall at 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
> > <llvm-commits at lists.llvm.org <mailto:llvm-commits at 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
> >>     <reviews at reviews.llvm.org <mailto:reviews at 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
> >>
> >>      ; <label>: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
> >>
> >>      ; <label>:9:                                      ; preds = %0
> >>        call void @llvm.objc.autoreleasePoolPop(i8* %1)
> >>        %10 = load i1, i1* @deallocCalled, align 1
> >>        br i1 %10, label %12, label %11
> >>
> >>      ; <label>: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
> >>
> >>      ; <label>: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
> >>
> >>      ; <label>:9:                                      ; preds = %0
> >>        call void @llvm.objc.autoreleasePoolPop(i8* %1)
> >>        br i1 %7, label %11, label %10
> >>
> >>      ; <label>: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-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> >     https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191017/1321808d/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: AssociatedObject_O0_static.ll
Type: application/octet-stream
Size: 21805 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191017/1321808d/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: AssociatedObject_O0_nostatic.ll
Type: application/octet-stream
Size: 21808 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191017/1321808d/attachment-0003.obj>


More information about the cfe-commits mailing list