<div dir="ltr">Please see updated <a href="https://reviews.llvm.org/D69690" target="_blank">D69690</a>.<div><br></div><div>Thanks,<br>Alina</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Nov 1, 2019 at 2:58 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>
I think the assumption that intrinsics can't read any global is, indeed, <br>
the problem here. `@llvm.objc.autoreleasePoolPop` may call any number <br>
of `-dealloc` methods with arbitrary side effects (though if they cause <br>
resurrection then that's undefined behaviour), so there should be no <br>
assumptions about the memory that it will read and write.<br>
<br>
This check looks wrong, the documentation in Intrinsics.td states:<br>
<br>
> By default, intrinsics are assumed to have side<br>
> effects, so this property is only necessary if you have defined one of<br>
> the memory properties listed above.<br>
<br>
Looking at the other intrinsics, the ones that don't touch memory <br>
(mostly?) seem to have the IntrNoMem attribute set. I think that should <br>
GlobalsAA should probably check that intrinsics either have that <br>
attribute set or one of the others that restricts the memory it can <br>
touch (e.g. IntrArgMemOnly).<br>
<br>
David<br>
<br>
On 31/10/2019 23:55, Alina Sbirlea wrote:<br>
> Following up on this, AFAICT this is working as intended on the LLVM side.<br>
> <br>
> The different decision is made in GlobalsAA + MemoryDependencyAnalysis.<br>
> IIUC, the logic there is along the lines of: if I have a global G that's <br>
> "internal" ("static" in C), and an intrinsic method M, then M cannot <br>
> read from or write to G. In the LLVM IR for the test, @deallocCalled is <br>
> an internal global, @llvm.objc.autoreleasePoolPop is an intrinsic, hence <br>
> @llvm.objc.autoreleasePoolPop cannot read or write @deallocCalled.<br>
> <br>
> Please note however that there are a couple of things that I found and <br>
> intend to fix in GlobalsAA, but they do not seem to affect this case.<br>
> <br>
> The one problem in particular that I thought was relevant here is that <br>
> the "MayReadAnyGlobal" is not set on a path in <br>
> GlobalsAAResult::AnalyzeCallGraph, but should apply when the function is <br>
> not an intrinsic (judging by the other code paths). If <br>
> @llvm.objc.autoreleasePoolPop was not an intrinsic, then with the fix in <br>
> D69690 <<a href="https://reviews.llvm.org/D69690" rel="noreferrer" target="_blank">https://reviews.llvm.org/D69690</a>> would resolve this miscompile.<br>
> Perhaps we should not rely on the assumption that intrinsics are <br>
> restricted to this behavior in GlobalsAA?<br>
> <br>
> Best,<br>
> Alina<br>
> <br>
> <br>
> On Thu, Oct 17, 2019 at 3:00 PM Alina Sbirlea <<a href="mailto:alina.sbirlea@gmail.com" target="_blank">alina.sbirlea@gmail.com</a> <br>
> <mailto:<a href="mailto:alina.sbirlea@gmail.com" target="_blank">alina.sbirlea@gmail.com</a>>> wrote:<br>
> <br>
> I only got a chance to look more into this now. I can reproduce it<br>
> with re-inserting the "static".<br>
> <br>
> The miscompile is not related to MemorySSA, i.e. disabling all uses<br>
> of MemorySSA doesn't help.<br>
> It appears to be a GVN bug, but I haven't tracked it further than this.<br>
> <br>
> To repro, see attached .ll files. The only difference is the global<br>
> variable being static or not, which in IR translates to internal or<br>
> dso_local.<br>
> A simple `opt -O3 AssociatedObject_O0_*.ll` shows the difference you<br>
> mention.<br>
> <br>
> Reducing the pipeline, I believe the result after the following<br>
> command is incorrect.<br>
> `opt -globals-aa -gvn AssociatedObject_O0_*.ll`<br>
> I'll need to dig deeper to confirm and track down the bug inside the<br>
> pass.<br>
> <br>
> <br>
> Thanks,<br>
> Alina<br>
> <br>
> <br>
> On Mon, Sep 30, 2019 at 4:30 AM David Chisnall<br>
> <<a href="mailto:David.Chisnall@cl.cam.ac.uk" target="_blank">David.Chisnall@cl.cam.ac.uk</a> <mailto:<a href="mailto:David.Chisnall@cl.cam.ac.uk" target="_blank">David.Chisnall@cl.cam.ac.uk</a>>><br>
> wrote:<br>
> <br>
> Hi,<br>
> <br>
> Yes, I believe it does still reproduce (at least, with the most<br>
> recent<br>
> build that I tried). We worked around the clang bug to make the<br>
> 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<br>
> case to<br>
> unconditionally failing the assert immediately after the pop,<br>
> 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<br>
> 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><br>
> <mailto:<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br>
> <mailto:<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <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 :<br>
> Intrinsic<[],<br>
> > [llvm_ptr_ty]>;<br>
> ><br>
> ><br>
> > which (I believe) means it has unmodelled side effects so<br>
> 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<br>
> 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><br>
> <mailto:<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>><br>
> <mailto:<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a><br>
> <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<br>
> revision that<br>
> >> introduced the regression in the GNUstep Objective-C<br>
> runtime test<br>
> >> suite that I reported on the list a few weeks ago. In<br>
> this is the<br>
> >> test (compiled with `-fobjc-runtime=gnustep-2.0 -O3` and<br>
> an ELF<br>
> >> triple):<br>
> >><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<br>
> that the<br>
> >> second load of `deallocCalled` is redundant. The code<br>
> goes from:<br>
> >><br>
> >> %7 = load i1, i1* @deallocCalled, align 1<br>
> >> br i1 %7, label %8, label %9<br>
> >><br>
> >> ; <label>:8: ;<br>
> preds = %0<br>
> >> call void @__assert(i8* getelementptr inbounds ([5 x<br>
> i8], [5 x<br>
> >> i8]* @__func__.main, i64 0, i64 0), i8* getelementptr<br>
> 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,<br>
> i64 0, i64<br>
> >> 0)) #5<br>
> >> unreachable<br>
> >><br>
> >> ; <label>:9: ;<br>
> 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: ;<br>
> preds = %9<br>
> >> call void @__assert(i8* getelementptr inbounds ([5 x<br>
> i8], [5 x<br>
> >> i8]* @__func__.main, i64 0, i64 0), i8* getelementptr<br>
> 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,<br>
> 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: ;<br>
> preds = %0<br>
> >> call void @__assert(i8* getelementptr inbounds ([5 x<br>
> i8], [5 x<br>
> >> i8]* @__func__.main, i64 0, i64 0), i8* getelementptr<br>
> 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,<br>
> i64 0, i64<br>
> >> 0)) #5<br>
> >> unreachable<br>
> >><br>
> >> ; <label>:9: ;<br>
> preds = %0<br>
> >> call void @llvm.objc.autoreleasePoolPop(i8* %1)<br>
> >> br i1 %7, label %11, label %10<br>
> >><br>
> >> ; <label>:10: ;<br>
> preds = %9<br>
> >> call void @__assert(i8* getelementptr inbounds ([5 x<br>
> i8], [5 x<br>
> >> i8]* @__func__.main, i64 0, i64 0), i8* getelementptr<br>
> 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,<br>
> i64 0, i64<br>
> >> 0)) #5<br>
> >> unreachable<br>
> >><br>
> >> Later optimisations then determine that, because the<br>
> assert does<br>
> >> not return, the only possible value for %7 is false and<br>
> cause the<br>
> >> second assert to fire unconditionally.<br>
> >><br>
> >> It appears that we are not correctly modelling the side<br>
> effects of<br>
> >> the `llvm.objc.autoreleasePoolPop` intrinsic, but it's not<br>
> >> entirely clear why not. The same test compiled for the<br>
> 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<br>
> correctly,<br>
> >> but with this change the optimisers are assuming that no<br>
> 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<br>
> anything<br>
> >> wrong, so I still suspect that there is a MemorySSA bug<br>
> that this<br>
> >> has uncovered, rather than anything wrong in this series of<br>
> >> commits. Any suggestions as to where to look would be<br>
> 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><br>
> <mailto:<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br>
> <mailto:<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <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>
> <br>
</blockquote></div>