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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Just some minor notes.



================
Comment at: llvm/docs/LangRef.rst:19589
+
+      declare void @llvm.experimental.noalias.scope.decl(metadata !id.scope)
+
----------------
Maybe name it `!id.scope.list` to make it clear that it's not a single scope?


================
Comment at: llvm/docs/LangRef.rst:19612
+noalias scope is declared. When the intrinsic is duplicated, also a decision
+must be made about the scope: depending on the reason of the duplication,
+sometimes the scope must be duplicated as well.
----------------
", also a decision must be made about the scope" -> ", a decision must also be made about the scope"


================
Comment at: llvm/docs/LangRef.rst:19616
+For example, when the intrinsic is used inside a loop body, and that loop is
+unrolled, it is an indication that the associate noalias scope must also
+be duplicated. Otherwise, the noalias property it signifies would spill
----------------
associate -> associated


================
Comment at: llvm/docs/LangRef.rst:19637
+    %a.inc = getelementptr inbounds i8, i8* %a, i64 1
+    %b.inc = getelementptr inbounds i8, i8* %a, i64 1
+    %cond = call i1 @cond()
----------------
The %a here should be %b.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:550
+    : DefaultAttrsIntrinsic<[], [llvm_metadata_ty],
+        [IntrInaccessibleMemOnly]>; // blocks LICM and some more
+
----------------
At the technical call @jdoerfert also mentioned the possibility of marking it `noduplicate`, but after looking at some of the usages of `cannotDuplicate()` I'm pretty sure this will cause more issues than it will solve.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6407
   case Intrinsic::stackprotector: {
-    // Emit code into the DAG to store the stack guard onto the stack.
+    // Emit code into the DAG tostore the stack guard onto the stack.
     MachineFunction &MF = DAG.getMachineFunction();
----------------
Spurious change.


================
Comment at: llvm/lib/IR/IRBuilder.cpp:461
+      M, Intrinsic::experimental_noalias_scope_decl, {});
+  return createCallHelper(FnIntrinsic, Ops, this);
+}
----------------
I believe you can just pass `{Scope}` here and drop the SmallVector.


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

https://reviews.llvm.org/D93039