[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