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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.

A few minor wording suggestions and comments, nothing that would require another round of review. Adopt my suggestions as you see fit, add TODOs where it makes sense and something should be investigated in the future. LGTM



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



================
Comment at: llvm/docs/LangRef.rst:19613
+also be made about the scope: depending on the reason of the duplication,
+sometimes the scope must be duplicated as well.
+
----------------
Same wording as above.


================
Comment at: llvm/docs/LangRef.rst:19615-19618
+For example, when the intrinsic is used inside a loop body, and that loop is
+unrolled, it is an indication that the associated noalias scope must also
+be duplicated. Otherwise, the noalias property it signifies would spill
+across loop iterations, whereas it was only valid within a single iteration.
----------------



================
Comment at: llvm/docs/LangRef.rst:19622
+                
+  ; 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.
----------------



================
Comment at: llvm/docs/LangRef.rst:19650-19652
+are possible, but one should never dominate another. Violations are pointed out
+by the verifier and indicate bugs in transformation passes or input to the
+parser.
----------------



================
Comment at: llvm/docs/LangRef.rst:19653
+parser.
+
 
----------------
Nit: `Version` or `version`, not both.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:550
+    : DefaultAttrsIntrinsic<[], [llvm_metadata_ty],
+        [IntrInaccessibleMemOnly]>; // blocks LICM and some more
+
----------------
nikic wrote:
> 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.
Interesting, at some point I'd like to hear more. Let's go with `IntrInaccessibleMemOnly` for now but we should be on the lookout for better solutions here. This is not the only intrinsic and the less restrictive our annotations are the better.


================
Comment at: llvm/lib/IR/Verifier.cpp:5571
+
+  llvm::sort(NoAliasScopeDecls, Compare);
+
----------------
If this sorts based on pointer values it is non-deterministic. While potentially OK in the verifier, I'd recommend a TODO. We should sort by "metadata ids" (!0, !1, ...) or something that is deterministic. 


================
Comment at: llvm/lib/IR/Verifier.cpp:5597
+    ItCurrent = ItNext;
+  }
+}
----------------
FWIW, I would set this up the other way around, maybe worth considering but not strictly necessary:

1) Collect all scopes from the intrinsics in a map<Scope, SmallVector<Instruction>>
2) For each scope, take the last and all other instructions in the vector and check dominance.
3) If there is more than one, drop the last instruction and go to 2.




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

https://reviews.llvm.org/D93039