[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.

In http://reviews.llvm.org/D15124#302904, @lvoufo wrote:

>




> For the time being, it seems more effective to take the current approach. But perhaps I am missing something about LLVM's print analyses... I am also going from this notion that test cases are about confidence more than completeness. So, it is perfectly okay to center them around specific use cases showing the end results of the print messages (i.e. load removal) rather than the print messages themselves(?).


What I'm proposing is to make this analysis pass runnable via opt.  Kind of like the other analyses in lib/Analysis.  So, we could run, for instance, 'opt -invariant-info-marker file.ll' and it would print the invariant ranges in file.ll.  From this we can then create tests specifically for the pass, without having to go through another pass.


================
Comment at: lib/Analysis/InvariantInfo.cpp:110
@@ +109,3 @@
+/// corresponding address as "written" 'writeonce', and thus treatable as not
+/// pointing to constant memory. If the intrinsic instruction is an
+/// invariant_end instead, then mark the address as no longer "written"
----------------
I'm confused by this comment.  When we find an invariant_start, shouldn't the address be considered as pointing to constant memory?

================
Comment at: lib/Analysis/InvariantInfo.cpp:125
@@ +124,3 @@
+      InvInfo.UnsetStartInstruction(Addr);
+    return true;
+  }
----------------
What does the return value indicate?

================
Comment at: lib/Analysis/InvariantInfo.cpp:138
@@ +137,3 @@
+bool llvm::BackwardScanInvariantIntrinsic(const IntrinsicInst *II,
+                                    const PreserveInvariantInfoRAII &Preserved,
+                                          InvariantInfo *InvInfo) {
----------------
This function is supposedly doing a scan backwards, but all it really seems to do is decide whether II is an invariant_start or an invariant_end.  Does this need to be renamed?  I'm not really sure what this is doing.

================
Comment at: lib/Analysis/InvariantInfo.cpp:139
@@ +138,3 @@
+                                    const PreserveInvariantInfoRAII &Preserved,
+                                          InvariantInfo *InvInfo) {
+  if (II->getIntrinsicID() == Intrinsic::invariant_start) {
----------------
Formatting is odd here.  Could you run clang-format over the file?


http://reviews.llvm.org/D15124