Re: [PATCH] D55802: Change CGObjC to use objc intrinsics instead of runtime methods

[David Chisnall via cfe-commits]

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-comm...@lists.llvm.org <mailto:llvm-comm...@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
    <revi...@reviews.llvm.org <mailto:revi...@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-comm...@lists.llvm.org <mailto:llvm-comm...@lists.llvm.org>
    https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits