[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/FunctionAttrs.cpp:1519
 
-  // Non volatile memset/memcpy/memmoves are nosync
-  // NOTE: Only intrinsics with volatile flags should be handled here.  All
----------------
reames wrote:
> This removal breaks our ability to infer nosync on a non-volatile memset, exactly as the comment describes.  There should be a test for this, if there isn't, let me know and I'll add one.
I think what happened is:

MemXXX was not marked nosync before, we had to derive it explicitly. To be correct, we checked for the volatile flag. Here and in the Attributor code above.
Then the DefaultAttrsIntrinsic change came and memXXX was marked nosync regardless of the volatile flag. That is wrong as I stated in my comment above. In the Attributor code above this patch now filters the wrongly annotated calls, in this code it doesn't and they just split through and propagate the wrong nosync flag further. Intrinsics.td need to be changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102295