[PATCH] D59065: Add ptrmask intrinsic

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: llvm/docs/LangRef.rst:16397
+must be zero when accessing it, because of ABI alignment requirements or
+a restriction of the meaningful bits through the data layout. This can be used
+to strip data from tagged pointers without using ptrtoint/inttoptr and allows
----------------
"a restriction of the meaningful bits through the data layout" - I'm not sure what this is supposed to mean. I guess that you're referring to higher-order bits which are not actually used to form valid addresses (e.g., if a system has 64-bit pointers but only uses 48 bits in practice).


================
Comment at: llvm/docs/LangRef.rst:16404
+
+The mask argument must only zero out bits that are not part of the relevant bits
+of the pointer. Passing masks that zero out relevant bits of the pointer is
----------------
Please specifically specify how the mask is applied. It is currently unclear whether this is computing (P & mask) or (P & ~mask) or something else.


================
Comment at: llvm/docs/LangRef.rst:16406
+of the pointer. Passing masks that zero out relevant bits of the pointer is
+undefined behavior. The relevant bits of a pointer include the bitwidth specified
+in the data layout minus the bits required by the ABI alignment requirement.
----------------
aqjune wrote:
> Defining this case as undefined behavior may block code motion.
> For example (which does LICM):
> ```
> for (..) {
>   if (cond) break;
>   p = llvm.ptrmask(p0, mask)
>   use(p)
> }
> =>
> p = llvm.ptrmask(p0, mask)
> for (..) {
>   if (cond) break;
>   use(p)
> }
> ```
> If cond is false and mask is invalid, the source program has no UB (because the call is never executed), but the optimized program raises UB, which is incorrect.
> Defining p as `poison` in this case would resolve the problem.
Hopefully, the UB would occur only if the resulting pointer were accessed. If an invalid mask is UB, this will prevent the intrinsic from being hoisted, and that seems undesirable. Regardless, please remove the UB-related text from here. Once we specify what the intrinsic does, the rest follows from pre-existing semantics and trying to specify all of the relevant conditions here seems unlikely to be useful.


================
Comment at: llvm/docs/LangRef.rst:16408
+in the data layout minus the bits required by the ABI alignment requirement.
+The intrinsic will get lowered to a bitwise and instruction or equivalent. Both
+the returned pointer and the first argument point to the same underlying object.
----------------
I think that it's best to specify only the effect. Maybe something like: The effect of this intrinsic is to ensure that ptrtoint(ptrmask(p, m)) == (ptrtoint(p) & m).


================
Comment at: llvm/docs/LangRef.rst:16409
+The intrinsic will get lowered to a bitwise and instruction or equivalent. Both
+the returned pointer and the first argument point to the same underlying object.
+
----------------
... and the first argument are based on the same underlying object. (ref. the "based on" terminology from the "Pointer Aliasing Rules" section).


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

https://reviews.llvm.org/D59065