[PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods
David Chisnall via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 1 02:58:36 PDT 2019
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
> >
>
More information about the llvm-commits
mailing list