<div dir="ltr">Hi David,<div><br></div><div>Does this still reproduce?</div><div>I'm seeing the load after the call to autoreleasePoolPop at ToT, but perhaps I'm not using the proper flags?</div><div>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.</div><div><br></div><div>Thanks,</div><div>Alina</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Feb 24, 2019 at 9:56 AM Pete Cooper via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;">Hey David<div><br></div><div>Thanks for letting me know, and analysing it this far!</div><div><br></div><div>I also can't see anything wrong with the intrinsic. Its just defined as:</div><div><br></div><blockquote style="margin:0px 0px 0px 40px;border:none;padding:0px"><div>def int_objc_autoreleasePoolPop : Intrinsic<[], [llvm_ptr_ty]>;</div></blockquote><div><br></div><div>which (I believe) means it has unmodelled side effects so it should have been fine for your example.</div><div><br></div><div>I'll try build the same file you did and see if I can reproduce.</div><div><br></div><div>Cheers,</div><div>Pete<br><div><br><blockquote type="cite"><div>On Feb 24, 2019, at 7:48 AM, David Chisnall via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:</div><br class="gmail-m_-8175512114545570343Apple-interchange-newline"><div><div>theraven added a comment.<br>Herald added a project: LLVM.<br><br>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):<br><br><a href="https://github.com/gnustep/libobjc2/blob/master/Test/AssociatedObject.m" target="_blank">https://github.com/gnustep/libobjc2/blob/master/Test/AssociatedObject.m</a><br><br>After this change, Early CSE w/ MemorySSA is determining that the second load of `deallocCalled` is redundant. The code goes from:<br><br> %7 = load i1, i1* @deallocCalled, align 1<br> br i1 %7, label %8, label %9<br><br> ; <label>:8: ; preds = %0<br> 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<br> unreachable<br><br> ; <label>:9: ; preds = %0<br> call void @llvm.objc.autoreleasePoolPop(i8* %1)<br> %10 = load i1, i1* @deallocCalled, align 1<br> br i1 %10, label %12, label %11<br><br> ; <label>:11: ; preds = %9<br> 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<br> unreachable<br><br>to:<br><br> %7 = load i1, i1* @deallocCalled, align 1<br> br i1 %7, label %8, label %9<br><br> ; <label>:8: ; preds = %0<br> 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<br> unreachable<br><br> ; <label>:9: ; preds = %0<br> call void @llvm.objc.autoreleasePoolPop(i8* %1)<br> br i1 %7, label %11, label %10<br><br> ; <label>:10: ; preds = %9<br> 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<br> unreachable<br><br>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.<br><br>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).<br><br>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.<br><br><br>Repository:<br> rL LLVM<br><br>CHANGES SINCE LAST ACTION<br> <a href="https://reviews.llvm.org/D55802/new/" target="_blank">https://reviews.llvm.org/D55802/new/</a><br><br><a href="https://reviews.llvm.org/D55802" target="_blank">https://reviews.llvm.org/D55802</a><br><br><br><br></div></div></blockquote></div><br></div></div>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>