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

[Alina Sbirlea via cfe-commits]

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> 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> 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
> 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/20190910/98fa35f4/attachment-0001.html>