Following up on this, AFAICT this is working as intended on the LLVM side.
The different decision is made in GlobalsAA + MemoryDependencyAnalysis.
IIUC, the logic there is along the lines of: if I have a global G that's
"internal" ("static" in C), and an intrinsic method M, then M cannot
read from or write to G. In the LLVM IR for the test, @deallocCalled is
an internal global, @llvm.objc.autoreleasePoolPop is an intrinsic, hence
@llvm.objc.autoreleasePoolPop cannot read or write @deallocCalled.
Please note however that there are a couple of things that I found and
intend to fix in GlobalsAA, but they do not seem to affect this case.
The one problem in particular that I thought was relevant here is that
the "MayReadAnyGlobal" is not set on a path in
GlobalsAAResult::AnalyzeCallGraph, but should apply when the function is
not an intrinsic (judging by the other code paths). If
@llvm.objc.autoreleasePoolPop was not an intrinsic, then with the fix in
D69690 <https://reviews.llvm.org/D69690> would resolve this miscompile.
Perhaps we should not rely on the assumption that intrinsics are
restricted to this behavior in GlobalsAA?
Best,
Alina
On Thu, Oct 17, 2019 at 3:00 PM Alina Sbirlea <alina.sbir...@gmail.com
<mailto:alina.sbir...@gmail.com>> wrote:
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 <mailto: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>
<mailto: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>
<mailto: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>
<mailto:llvm-comm...@lists.llvm.org
<mailto:llvm-comm...@lists.llvm.org>>
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
