[PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods
Alina Sbirlea via llvm-commits
llvm-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/llvm-commits/attachments/20191017/1321808d/attachment.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/llvm-commits/attachments/20191017/1321808d/attachment.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/llvm-commits/attachments/20191017/1321808d/attachment-0001.obj>
More information about the llvm-commits
mailing list