<div dir="ltr">I only got a chance to look more into this now. I can reproduce it with re-inserting the "static".<div><br><div>The miscompile is not related to MemorySSA, i.e. disabling all uses of MemorySSA doesn't help.</div><div>It appears to be a GVN bug, but I haven't tracked it further than this.</div><div><br></div><div>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.<br></div><div>A simple `opt -O3 AssociatedObject_O0_*.ll` shows the difference you mention.</div><div><br></div><div>Reducing the pipeline, I believe the result after the following command is incorrect.</div><div>`opt -globals-aa -gvn AssociatedObject_O0_*.ll` </div><div>I'll need to dig deeper to confirm and track down the bug inside the pass.</div><div><br></div><div><br></div><div>Thanks,</div><div>Alina</div>
<div><br></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Sep 30, 2019 at 4:30 AM David Chisnall <<a href="mailto:David.Chisnall@cl.cam.ac.uk">David.Chisnall@cl.cam.ac.uk</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">Hi,<br>
<br>
Yes, I believe it does still reproduce (at least, with the most recent <br>
build that I tried). We worked around the clang bug to make the test pass:<br>
<br>
<a href="https://github.com/gnustep/libobjc2/commit/eab35fce379eb6ab1423dbb6d7908cb782f13458#diff-7f1eea7fdb5c7c3bf21f08d1cfe98a19" rel="noreferrer" target="_blank">https://github.com/gnustep/libobjc2/commit/eab35fce379eb6ab1423dbb6d7908cb782f13458#diff-7f1eea7fdb5c7c3bf21f08d1cfe98a19</a><br>
<br>
Reintroducing the 'static' on deallocCalled reduces the test case to <br>
unconditionally failing the assert immediately after the pop, and DCEs <br>
the rest of the code.<br>
<br>
David<br>
<br>
On 11/09/2019 01:17, Alina Sbirlea wrote:<br>
> Hi David,<br>
> <br>
> Does this still reproduce?<br>
> I'm seeing the load after the call to autoreleasePoolPop at ToT, but <br>
> perhaps I'm not using the proper flags?<br>
> I only checked out the github repo and did "clang <br>
> -fobjc-runtime=gnustep-2.0 -O3 -emit-llvm -S <br>
> libobjc2/Test/AssociatedObject.m" with a ToT clang.<br>
> <br>
> Thanks,<br>
> Alina<br>
> <br>
> <br>
> On Sun, Feb 24, 2019 at 9:56 AM Pete Cooper via llvm-commits <br>
> <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a> <mailto:<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>>> wrote:<br>
> <br>
> Hey David<br>
> <br>
> Thanks for letting me know, and analysing it this far!<br>
> <br>
> I also can't see anything wrong with the intrinsic. Its just<br>
> defined as:<br>
> <br>
> def int_objc_autoreleasePoolPop : Intrinsic<[],<br>
> [llvm_ptr_ty]>;<br>
> <br>
> <br>
> which (I believe) means it has unmodelled side effects so it should<br>
> have been fine for your example.<br>
> <br>
> I'll try build the same file you did and see if I can reproduce.<br>
> <br>
> Cheers,<br>
> Pete<br>
> <br>
>> On Feb 24, 2019, at 7:48 AM, David Chisnall via Phabricator<br>
>> <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a> <mailto:<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>>> wrote:<br>
>><br>
>> theraven added a comment.<br>
>> Herald added a project: LLVM.<br>
>><br>
>> After some bisection, it appears that this is the revision that<br>
>> introduced the regression in the GNUstep Objective-C runtime test<br>
>> suite that I reported on the list a few weeks ago. In this is the<br>
>> test (compiled with `-fobjc-runtime=gnustep-2.0 -O3` and an ELF<br>
>> triple):<br>
>><br>
>> <a href="https://github.com/gnustep/libobjc2/blob/master/Test/AssociatedObject.m" rel="noreferrer" 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<br>
>> 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<br>
>> i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds<br>
>> ([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 26, i8*<br>
>> getelementptr inbounds ([15 x i8], [15 x i8]* @.str.1, i64 0, i64<br>
>> 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<br>
>> i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds<br>
>> ([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 29, i8*<br>
>> getelementptr inbounds ([14 x i8], [14 x i8]* @.str.2, i64 0, i64<br>
>> 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<br>
>> i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds<br>
>> ([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 26, i8*<br>
>> getelementptr inbounds ([15 x i8], [15 x i8]* @.str.1, i64 0, i64<br>
>> 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<br>
>> i8]* @__func__.main, i64 0, i64 0), i8* getelementptr inbounds<br>
>> ([27 x i8], [27 x i8]* @.str, i64 0, i64 0), i32 29, i8*<br>
>> getelementptr inbounds ([14 x i8], [14 x i8]* @.str.2, i64 0, i64<br>
>> 0)) #5<br>
>> unreachable<br>
>><br>
>> Later optimisations then determine that, because the assert does<br>
>> not return, the only possible value for %7 is false and cause the<br>
>> second assert to fire unconditionally.<br>
>><br>
>> It appears that we are not correctly modelling the side effects of<br>
>> the `llvm.objc.autoreleasePoolPop` intrinsic, but it's not<br>
>> entirely clear why not. The same test compiled for the macos<br>
>> runtime does not appear to exhibit the same behaviour. The<br>
>> previous revision, where we emitted a call to<br>
>> `objc_autoreleasePoolPop` and not the intrinsic worked correctly,<br>
>> but with this change the optimisers are assuming that no globals<br>
>> can be modified across an autorelease pool pop operation (at<br>
>> least, in some situations).<br>
>><br>
>> Looking at the definition of the intrinsic, I don't see anything<br>
>> wrong, so I still suspect that there is a MemorySSA bug that this<br>
>> has uncovered, rather than anything wrong in this series of<br>
>> 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/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D55802/new/</a><br>
>><br>
>> <a href="https://reviews.llvm.org/D55802" rel="noreferrer" target="_blank">https://reviews.llvm.org/D55802</a><br>
>><br>
>><br>
>><br>
> <br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a> <mailto:<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>
> <br>
</blockquote></div>