[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-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
>