[PATCH] D102295: [FunctionAttr][Attributor] Cleanup `nosync` checks of memory intrinsics

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1337
+      if (isSyncIntrinsic(&I))
+        return false;
 
----------------
This doesn't make sense to me now. I think I figured out my problem (below) but I wanted to keep my reasoning:
So, a `memintrinsic` is either `nosync` or it is not. Let's play this through:
1) We add `nosync` to the ones that are not volatile we don't need this because of the check below. This doesn't seem to be the case right now.
2) We don't add `nosync` to any `memintrinsics` and we need to check them explicitly, this is what the old code was supposed to do (I imagine) but the new code doesn't do that. That said, we add `nosync` so it's actually 3).
3) We add `nosync` to all `memintrinsics` regardless if they are volatile or not. This seems to be plain wrong. The new code will filter the ones out that we wrongly annotated so the error does not propagate. I doubt this is the right strategy though. We should go with 1) or 2) instead.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102295