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.chisn...@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-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 > > >
AssociatedObject_O0_static.ll
Description: Binary data
AssociatedObject_O0_nostatic.ll
Description: Binary data
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits