[PATCH] D69542: Full Restrict Support - single patch

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere added a comment.

Hi Ralf, thanks for your feedback !

This convenience patch is made available to show what the total picture looks like. The current set of extracted patches (`ptr_provenance` and `unknown_provenance`) that is available for review can be found here: D104268 <https://reviews.llvm.org/D104268>  (and in the 'stack')

Any help in reviewing and feedback is appreciated !

Thanks,

Jeroen Dobbelaere



================
Comment at: llvm/docs/LangRef.rst:9966-9971
+The optional ``ptr_provenance`` argument specifies the noalias chain of the
+pointer operand. It has the same type as the pointer operand. Together with the
+``!noalias`` metadata on the instruction, and the ``llvm.provenance.noalias``,
+``llvm.noalias.arg.guard`` intrinsics in the chain, this is used to deduce if
+two load/store instructions may or may not alias. (See `Scoped NoAlias Related
+Intrinsics`_)
----------------
RalfJung wrote:
> Rust and C also have a notion of "pointer provenance", but it is subtly difference: provenance in those languages is something that flows with pointer values. IOW, a value of pointer type consists of some address in memory, plus some "provenance" metadata. LLVM also has that kind of provenance, it is needed e.g. to explain some of the behavior of `getelementptr`. (A pointer returned by `getelementptr` without inbounds can go out-of-bounds but must not be used to access memory outside the bounds of the allocation it started with. In other words, the pointer "remembers" the allocation it is associated with in some way that is separate from the integer address it points to.)
> 
> Is it a good idea to use the same term for this slightly different concept? A `load` operation already receives provenance from its pointer argument, and now with this patchset it *also* receives something else, but also called provenance, via a separate argument.
This is rather an extension of this concept than a different concept: the patches extend the notion of a pointer that represents a value + provenance. For the load/store instructions, when no `ptr_provenance` is present, the single pointer represents both the value and provenance (and `getPtrProvenance() will return this value). When the `ptr_provenance` argument has been set it means that for these instructions, the `getPointerOperand` now only points to the value. The `getPtrPtrProvenance()` and `getOptionalPtrProvenance()` will now return the separate `ptr_provenance`.

It is still assumed that a single pointer contains both value and provenance.


================
Comment at: llvm/docs/LangRef.rst:23372
 
+Scoped NoAlias Related Intrinsics
+---------------------------------
----------------
RalfJung wrote:
> From what I was able to see (but maybe I missed something), the docs do not say what exactly happens when these aliasing requirements are violated. I would expect that this results in immediate UB. During some [recent discussion](https://discourse.llvm.org/t/interaction-of-noalias-and-dereferenceable/66979), it was suggested that noalias-violating loads might return poison instead of causing UB, but [I think that proposal has problems](https://discourse.llvm.org/t/interaction-of-noalias-and-dereferenceable/66979/14). Either way though the docs should clearly state what the behavior is in that case.
This explanation is indeed missing. In practice, noalias-violating loads must be kept as ok. noalias-violating loads in combination with a noalias-violating store results in undefined behavior. The background for this is that: reorderings of a noalias-violating load/load should not harm. Reorderings of a noalias-violating store/store, load/store or store/load might result in unwanted results.



================
Comment at: llvm/docs/LangRef.rst:23401
+
+A detailed description of these intrinsics and how the work is explained in
+::doc::`Restrict and NoAlias Information in LLVM <NoAliasInfo>`
----------------
RalfJung wrote:
> should probably be: how "they" work
Indeed


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

https://reviews.llvm.org/D69542