[PATCH] D15124: Use @llvm.invariant.start/end intrinsics to extend the meaning of basic AA's pointsToConstantMemory(), for GVN-based load elimination purposes [Local objects only]

[Diego Novillo via llvm-commits]

dnovillo added a comment.

I've just started looking at this, so these first few comments are mostly for my benefit to make sure I'm understanding this.

Would it make sense to have tests that are purely around the InvariantInfo analysis?  This patch adds a user in AA and the test is on AA.  But if the analysis were to print its findings, we could have a test that simply checks that the analysis figures out what we are expecting without involving AA.


================
Comment at: include/llvm/Analysis/InvariantInfo.h:28
@@ +27,3 @@
+/// invariant intrinsic calls.
+class InvariantInfo {
+
----------------
Documentation does not match the class name.  I'd just get rid of the class name in the doc string.

================
Comment at: include/llvm/Analysis/InvariantInfo.h:32
@@ +31,3 @@
+  /// GlobalVariable or AllocaInst instance) to its matching intrisic_start
+  /// call. This is to be used by GVN, InstCombine, GlobalOpt and BasicAA
+  /// to process invariant intrinsics for purposes of eliminating redundant
----------------
Actually, this'd be used by *any* pass that needs to understand write-once objects, right?

================
Comment at: include/llvm/Analysis/InvariantInfo.h:48
@@ +47,3 @@
+/// PreserveInvariantInfoRAII -- An RAII object to save and restore information
+/// from processed invariant intrinsics.
+struct PreserveInvariantInfoRAII {
----------------
Likewise here.  No need to duplicate the class name in the docstring.

================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:687
@@ +686,3 @@
+static bool isInvariantIntrinsic(ImmutableCallSite CS) {
+  const IntrinsicInst *II = dyn_cast<IntrinsicInst>(CS.getInstruction());
+  return II && (II->getIntrinsicID() == Intrinsic::invariant_start ||
----------------
Should this be defined in lib/Analysis/InvariantInfo.cpp?

================
Comment at: lib/Analysis/InvariantInfo.cpp:60
@@ +59,3 @@
+  //       So, do not insert nullptr values.
+  if (IStart) {
+    bool Inserted = InvariantMarkers.insert(std::make_pair(Addr, IStart)).second;
----------------
Would it be better to have a remove() function in the interface, instead of making nullptr a magic value?


http://reviews.llvm.org/D15124