[PATCH] D93039: Introduce llvm.noalias.decl intrinsic

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere added inline comments.


================
Comment at: llvm/docs/LangRef.rst:19466
+
+      declare i8* @llvm.noalias.decl.XXX(<type>* %p.alloca, i64 <p.objId>, metadata !p.scope)
+
----------------
nikic wrote:
> The `p.alloca` and `p.objId` arguments don't make a lot of sense without the remainder of the full restrict patch. The objId refers to a notion that doesn't exist in current LLVM.
> 
> I think it would be better to make the intrinsics just
> ```
> declare void @llvm.noalias.decl(metadata !p.scope)
> ```
> to start with. Adding additional arguments via AutoUpgrade should be straightfoward down the line.
The initial recommendation during the AA tech call was to already add all the arguments even if they are not needed yet. I guess that omitting them for now could work. Do you know the exact policy for backwards compatibility ? (released vs unreleased)


================
Comment at: llvm/docs/LangRef.rst:19475
+certain ``alloca`` is associated to an object that contains one or more
+restrict pointers.
+
----------------
nikic wrote:
> We should probably avoid using "restrict" terminology on the LLVM IR level, as restrict is a C/C++ specific concept. Rather than "restrict scope" I would call this a "noalias scope".
> 
> On that note, I wonder whether calling this intrinsic `@llvm.noalias.scope.start` rather than `@llvm.noalias.decl` might make sense. (The name mirrors llvm.lifetime.start and llvm.invariant.start -- but then again, those also have an "end", so maybe the parallel does not make sense.)
At this level, we can indeed omit references to restrict. For the larger explanation (full restrict), imho, it makes sense to use restrict as an example for explaining all the logic.

For the intrinsic: it is not a 'start of a scope'. The intrinsic indicates the location (in the code) where the scope was declared. Instructions metadata are allowed to refer to that scope, even if the intrinsic is not dominating that instruction. Optimizations are allowed to move instructions that refer to this scope over the intrinsic.



================
Comment at: llvm/docs/LangRef.rst:19498
+metadata references. The format is identical to that required for ``noalias``
+metadata. This list must have exactly one element.
+
----------------
nikic wrote:
> If only a single scope can be listed, might it make sense to directly point to the scope, rather than a list with a single scope? Or is the idea here that this might be extended to declare multiple scopes at the same time in the future?
Yes (to the second): one of the ideas is that a future optimization might combine llvm.noalias.decl into a single instruction, referring to the union of the scopes.



================
Comment at: llvm/include/llvm/IR/Intrinsics.td:556
+                [llvm_anyptr_ty, llvm_anyint_ty, llvm_metadata_ty],
+                [IntrArgMemOnly]>; // ArgMemOnly: blocks LICM and some more
+
----------------
nikic wrote:
> The ArgMemOnly here seems a bit dubious. This means it can read/write the p.alloca argument, which I assume is not intended (even if it's unused now).
> 
> I guess we can't make this `NoMem`, because then it could simply be DCEd right? Maybe it should be InaccessibleMemOnly?
That might also work.


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

https://reviews.llvm.org/D93039