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

[Larisse Voufo via llvm-commits]

lvoufo added inline comments.

================
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"
----------------
dnovillo wrote:
> I'm confused by this comment.  When we find an invariant_start, shouldn't the address be considered as pointing to constant memory?
Oops. Typo. That's exactly what the comment is supposed to be saying.

================
Comment at: lib/Analysis/InvariantInfo.cpp:125
@@ +124,3 @@
+      InvInfo.UnsetStartInstruction(Addr);
+    return true;
+  }
----------------
dnovillo wrote:
> What does the return value indicate?
That an invariant  intrinsic was processed. Will add a comment.

================
Comment at: lib/Analysis/InvariantInfo.cpp:138
@@ +137,3 @@
+bool llvm::BackwardScanInvariantIntrinsic(const IntrinsicInst *II,
+                                    const PreserveInvariantInfoRAII &Preserved,
+                                          InvariantInfo *InvInfo) {
----------------
dnovillo wrote:
> 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.
This is to be called while scanning instructions backward to temporarily reset the mapping of invariant info so that function calls outside invariant regions can continue to clobber loads inside invariant regions. Yes, it should probably be renamed. But I'm thinking of getting read of it all together, in favor of a separate function or pass that processes invariant intrinsics independently of the order in which GVN processes all instructions.

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


http://reviews.llvm.org/D15124