[PATCH] D115274: [IR][RFC] Memory region declaration intrinsic

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/docs/LangRef.rst:20430-20433
+Note that the ``begin_offset`` is non-positive (``begin_offset s<= 0``)
+and ``end_offset`` is non-negative (``end_offset s>= 0``) by definition,
+otherwise the ``ptr`` lies outside of declared memory region,
+and the returned pointer is a :ref:`poison value <poisonvalues>`.
----------------
courbet wrote:
> I'm still confused as to why we need that restriction.
> 
> For example, why do we want to disallow the following:
> 
> ```
>   %p2 = getelementptr inbounds i32, i32* %p1, i64 -42
>   %p2_bounded = @llvm.memory.region.decl.p0i8(%p2, 42, 43)
>   %p3 = getelementptr inbounds i32, i32* %p2_bounded, i64 42
>   %v = load i32, i32* %p3, align 4
> ```
At least as defined, `%p2_bounded` would already be poison in that case, because `%p2_bounded` would be before the declared memory region. The fact that `%p3` would point back into the memory region doesn't matter in that case, because the pointer is already poison.


================
Comment at: llvm/docs/LangRef.rst:20474
+However, there are **no** additional constraints imposed on the
+non-:ref:`inbounds <getelementptr_inbounds>` addresses for this memory region,
+
----------------
Doesn't this sentence contradict the first sentence in "Semantics"? If you want to make a distinction between inbounds/non-inbounds, then I think you have to do that in terms of restricting the visible allocated object, rather than saying that any pointer based on it cannot be outside the range. That would mean that something like `%p = memory.region.decl(%p0, 8, 16)` would not be poison, though dereferencing it would be and doing an inbounds gep would be, while doing a non-inbounds gep by 8 and then dereferencing would be legal.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1186
+     llvm_i64_ty /*end_offset*/], [IntrNoMem, IntrSpeculatable,
+     Returned<ArgIndex<0>>, ReadNone<ArgIndex<0>>]>;
+
----------------
While technically correct, annotating it `Returned` means that the intrinsic is simply going to be optimized away. You want to drop `returned` here and instead add the intrinsic to `isIntrinsicReturningPointerAliasingArgumentWithoutCapturing()`, which handles various intrinsics of this kind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115274