[PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods
Alina Sbirlea via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 1 12:54:01 PDT 2019
Please see updated D69690 <https://reviews.llvm.org/D69690>.
Thanks,
Alina
On Fri, Nov 1, 2019 at 2:58 AM David Chisnall <David.Chisnall at cl.cam.ac.uk>
wrote:
> 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 <https://reviews.llvm.org/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 <alina.sbirlea at gmail.com
> > <mailto:alina.sbirlea at gmail.com>> 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.Chisnall at cl.cam.ac.uk <mailto: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>
> > <mailto: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>
> > <mailto: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>
> > <mailto: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/20191101/478002f9/attachment.html>
More information about the llvm-commits
mailing list