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

[Jeroen Dobbelaere via Phabricator via llvm-commits]

jeroen.dobbelaere added a comment.

In D93039#2491603 <https://reviews.llvm.org/D93039#2491603>, @jdoerfert wrote:

> 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

Thanks for the feedback. I plan to do the small updates today and commit it so that we have a view on possible (unexpected) fallout.
@jdoerfert, @nikic, can you also spend some time on the other open reviews on the stack ?



================
Comment at: llvm/lib/IR/Verifier.cpp:5571
+
+  llvm::sort(NoAliasScopeDecls, Compare);
+
----------------
jdoerfert wrote:
> 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. 
Yes that's true. In the 'good' case, this will not make a difference of course. I'll add the TODO.


================
Comment at: llvm/lib/IR/Verifier.cpp:5597
+    ItCurrent = ItNext;
+  }
+}
----------------
jdoerfert wrote:
> 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.
> 
> 
My initial idea was to use a map of vectors as you suggest, but  I settled on doing a sort and avoiding a bunch of allocations. I am going to keep this for now. Having a nested for-loop does not make a big difference wrt complexity as we need to check the dominance for A vs B and for B vs A.



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

https://reviews.llvm.org/D93039