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

[Nikita Popov via Phabricator via llvm-commits]

nikic added reviewers: nikic, jdoerfert, asbirlea.
nikic 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)
+
----------------
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.


================
Comment at: llvm/docs/LangRef.rst:19475
+certain ``alloca`` is associated to an object that contains one or more
+restrict pointers.
+
----------------
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.)


================
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.
+
----------------
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?


================
Comment at: llvm/docs/LangRef.rst:19506
+care must be taken to duplicate and uniquify the associated scope when the loop
+is unrolled. Otherwise the restrict scope could spill across iterations.
 
----------------
I think adding an example for this intrinsic would be helpful. I'm thinking something along these lines:

```
; This examples shows two possible positions for noalias.decl:
; If it is outside the loop (version 1), then %a and %b are noalias across *all* iterations.
; If it is inside the loop (version 2), then %a and %b are noalias only within *one* iteration.
declare void @decl_in_loop(i8* %a.base, i8* %b.base) {
entry:
  ; call void @llvm.noalias.decl(metadata !2) ; Version 1: noalias.decl outside loop
  br label %loop

loop:
  %a = phi i8* [ %a.base, %entry ], [ %a.inc, %loop ]
  %b = phi i8* [ %b.base, %entry ], [ %b.inc, %loop ]
  ; call void @llvm.noalias.decl(metadata !2) ; Version 2: noalias.decl inside loop
  %val = load i8, i8* %a, !alias.scope !2
  store i8 %val, i8* %b, !noalias !2
  %a.inc = getelementptr inbounds i8, i8* %a, i64 1
  %b.inc = getelementptr inbounds i8, i8* %a, i64 1
  %cond = call i1 @cond()
  br i1 %cond, label %loop, label %exit

exit:
  ret void
}

!0 = !{!0} ; domain
!1 = !{!1, !0} ; scope
!2 = !{!1} ; scope list
```


================
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
+
----------------
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?


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

https://reviews.llvm.org/D93039