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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D123531#3446328 <https://reviews.llvm.org/D123531#3446328>, @efriedma wrote:

>> 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.
>
> I guess this is fine.  But please post a note on Discourse.

Will do as this is ready.



================
Comment at: llvm/include/llvm/IR/Intrinsics.td:428
+def int_objc_autoreleasePoolPop             : Intrinsic<[], [llvm_ptr_ty], [IntrNoSync, IntrNoCallback]>;
+def int_objc_autoreleasePoolPush            : Intrinsic<[llvm_ptr_ty], [], [IntrNoSync, IntrNoCallback]>;
 def int_objc_autoreleaseReturnValue         : Intrinsic<[llvm_ptr_ty],
----------------
efriedma wrote:
> In general, popping an autorelease pool deallocates objects, and deallocating objects can run arbitrary user code. So the new markings on int_objc_autoreleasePoolPop are wrong, I think.  Marking up int_objc_autoreleasePoolPush should be fine, though.
> 
> @rjmccall @ahatanak can you confirm?
I am waiting for ObjC people to look at this. I added the annotation based on two tests we otherwise fail, not based on the intrinsics (which I don't know).
Though, I might have misunderstood: `llvm/test/Analysis/GlobalsModRef/intrinsic_addressnottaken1.ll`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123531