[PATCH] D26485: Add IntrInaccessibleMemOnly property for intrinsics

[Hal Finkel via llvm-commits]

hfinkel added a comment.

> I considered adding this attribute to the @llvm.assume() intrinsic as Hal suggested in the comments on https://reviews.llvm.org/D26382, but that would have resulted in changes to aliasing behavior which is specifically tested for in test/Analysis/BasicAA/assume.ll and I am not familiar enough with that intrinsic to be comfortable judging whether or not that is acceptable. Consequently, I have no direct test for this change set. I am, of course, open to guidance.

I don't see why they should be different. Please feel free to update the test case and I'll review it -- or explain why the semantics are different.

> This code will not set the 'hasSideEffects' flag for intrinsics that use the IntrInaccessibleMemOnly property, but it currently does not set that flag for intrinsics that use the IntrArgMemOnly property. This seems wrong to me in both cases, but I decided to defer to the existing logic over my own judgment pending review.

hasSideEffects is somewhat misnamed. It really means "has otherwise-unmodeled side effects", so not setting it seems correct.



================
Comment at: include/llvm/IR/Intrinsics.td:48
+// accessible by the module being compiled. This is a weaker form of IntrNoMem.
+def IntrInaccessibleMemOnly : IntrinsicProperty;
+
----------------
We should go ahead and add InaccessibleMemOrArgMemOnly too while we're here.


Repository:
  rL LLVM

https://reviews.llvm.org/D26485