[PATCH] D15135: Extend the use of @llvm.invariant.start/end intrinsics for GVN-based load elimination purposes to also handle global variables.

[Nick Lewycky via llvm-commits]

nlewycky added inline comments.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2838-2839
@@ +2837,4 @@
+       CurInst != InstE; ++CurInst) {
+    if (isa<CallInst>(CurInst) || isa<InvokeInst>(CurInst)) {
+      CallSite CS(&*CurInst);
+
----------------
I think the usual way to write this is like:
  CallSite CS(&*CurInst);
  if (CS) {


================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2844
@@ +2843,3 @@
+
+      // Ignore debug info, inline asm, intrinsics, ...
+      if (isa<DbgInfoIntrinsic>(CS.getInstruction()) ||
----------------
DbgInfoIntrinsics are Intrinsics, you don't need to test for them separately. Also remove "..." because there aren't any more things being checked here.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2848
@@ +2847,3 @@
+          dyn_cast<IntrinsicInst>(CS.getInstruction())) {
+        ++CurInst;
+        continue;
----------------
Isn't this a double-increment? You'll skip CurInst here, then the for loop increment will skip whatever instruction comes after?

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2875-2880
@@ +2874,8 @@
+
+  // Scan the function's blocks to process invariant (start) intrinsics.
+  // This will mark writeonce global variables as written, and is necessary to
+  // do here because EvaluateBlock(), via EvaluateFunction() (below), could
+  // exit before processing the invariant intrinsic call, e.g., if a call to
+  // a function declaration that we cannot constant fold occurs before the
+  // intrinsic call.
+  processInvariantIntrinsics(InvInfo, F);
----------------
This doesn't explain what goes wrong if evaluation does fail part way through.

I think the point is that the optimization is for the case where we have an invariant.start with no invariant.end, and if EvaluateBlock terminates early we don't know whether there is an invariant.end matching our invariant.start.

But there's a much easier way to do this, the invariant.end must directly use its invariant.start ... so if the invariant.start has zero users then there is no end, otherwise there is an end. Now, I thought that optimization was already implemented which brings me to the test.

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2881
@@ +2880,3 @@
+  // intrinsic call.
+  processInvariantIntrinsics(InvInfo, F);
+
----------------
I'm deeply confused. This appears to populate InvInfo, but we never seem to use it for optimization?

Are you using this to populate InvInfo so that GVN can use it?

================
Comment at: test/Transforms/LoadElim/invariant.ll:39
@@ +38,3 @@
+
+define void @globalex() {
+entry:
----------------
This function is never called, and thus should never be examined by GlobalOpt's Evaluate calls at all, right?

I notice you're doing -globalopt -gvn. Please split GVN tests and GlobalOpt tests apart and add new changes testing globalopt only for changes to globalopt. If you need to verify that the combination of passes works well, you would add it to test/Transforms/PhaseOrdering, but I don't think that's necessary here.


http://reviews.llvm.org/D15135