[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 31 16:55:04 PDT 2019
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>
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> 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/20191031/b18adb95/attachment.html>
More information about the cfe-commits
mailing list