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

Tue Apr 12 10:43:27 PDT 2022

jdoerfert added a comment.

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

>> I'm not 100% sure if the issue underlying https://reviews.llvm.org/D115302 would be solved by this or not.
>
> You've included the `functions_without_nosync.ll` test from D115302 <https://reviews.llvm.org/D115302> which verifies that this patch fixes that issue, too. AFAICT, both patches are trying to address the same issue, but due to my lack of expertise, I was (overly?) cautious and trying to change as little as possible, and probably was doing it in a wrong place, too.
>
> 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`.

Yes. We need to properly make sync a first-class side-effect, get away from memory writes, etc.

> One concern raised on D115302 <https://reviews.llvm.org/D115302> was the impact on back-ends that have not been upgraded to use `DefaultIntrinsic`. For them this patch would prevent some optimizations. This could potentially be an issue for users who may use such intrinsic in the hot path and they will have no easy way to work around that, as intrinsic properties are not under user control. Should we consider having a temporary hidden escape hatch option to revert to the old behavior?

DefaultIntrinsic is available for a long time by now, people will not switch if they don't have to, nor will they revisit their intrinsics. It's still plenty of time till a release. Let's not add a hatch too early, give people a chance to realize and adjust.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123531