[PATCH] D123531: [GlobalsModRef][FIX] Ensure we honor synchronizing effects of intrinsics

Tue Apr 12 20:25:54 PDT 2022

jdoerfert added a comment.

In D123531#3446219 <https://reviews.llvm.org/D123531#3446219>, @tra wrote:

> We need to document how `nosync` (or lack of it) interacts with other attributes describing side effects. E.g. interaction between `IntrReadMem`, `IntrArgMemOnly ` and (lack of) `nosync`.

In D123531#3447021 <https://reviews.llvm.org/D123531#3447021>, @tra wrote:

> @jdoerfert:  A friendly nag to update intrinsic attribute docs. Looks like they didn't make it to the landed change.

You mean the above? That ain't something I can actually "just do". My opinion and the opinion of others are known to be different. What we need (in my opinion) is a major overhaul of our idea of side effects. We did a first step with termination but now we need to make "sync" and "write" different things. Even writing that up will take some time.

================
Comment at: llvm/lib/Analysis/GlobalsModRef.cpp:540
           FI.addModRefInfo(ModRefInfo::Ref);
-          if (!F->isIntrinsic() && !F->onlyAccessesArgMemory())
+          if (!F->onlyAccessesArgMemory() && MaySyncOrCallIntoModule(*F))
             // This function might call back into the module and read a global -
----------------
andrew.w.kaylor wrote:
> Should this also check onlyAccessesInaccessibleMemory() and onlyAccessesInaccessibleMemOrArgMem()? Same question for line 546 below.
Probably but unrelated. Feel free to propose a follow up.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123531