[PATCH] D115302: GlobalsModRef should treat functions w/o nosync conservatively.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D115302#3181440 <https://reviews.llvm.org/D115302#3181440>, @efriedma wrote:

> In D115302#3181294 <https://reviews.llvm.org/D115302#3181294>, @jdoerfert wrote:
>
>> In D115302#3181224 <https://reviews.llvm.org/D115302#3181224>, @tra wrote:
>>
>>> In D115302#3181180 <https://reviews.llvm.org/D115302#3181180>, @jdoerfert wrote:
>>>
>>>> EDIT:
>>>> Second thought, this does not fix my bug: https://lists.llvm.org/pipermail/llvm-dev/2021-December/154185.html
>>>
>>> I think it may. At least it does seem to preserve the load and produces identical code for both functions in my build. Unpatched LLVM does not.
>>>
>>>   Args: bin/opt -debug -aa-pipeline=basic-aa,globals-aa -passes=require<globals-aa>,function(gvn<pre;load-pre;split-backedge-load-pre;memdep>) -S
>>>   GVN iteration: 0
>>>   GVN: load i32 %r is clobbered by   call void @llvm.sync()
>>>   GVN iteration: 0
>>>   GVN: load i32 %r is clobbered by   call void @sync()
>>
>> But then I'm confused. The code that is patched here is generic for calls, no? I think this is a particular intrinsic issue.
>
> In general, GlobalsAA follows the call graph, and therefore conservatively assumes a call to an external function can call back into the current module.
>
> The callgraph code special-cases intrinsics, though.  Except for a few specific intrinsics, we assume intrinsics can't call back into the current module. See Intrinsic::isLeaf.



1. Then the intrinsics should be marked with the attribute that says they won't call back into the module: `nocallback` (which has no lang ref entry... https://reviews.llvm.org/D90275#2454912, anyway).
2. Not calling back into the module is not sufficient. Nosync is necessary, as noted here. I'm still somewhat unsure why it is checked for a call but OK, that might be the interface. I fear we forget to check it in other places too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115302/new/

https://reviews.llvm.org/D115302