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

[Larisse Voufo via llvm-commits]

lvoufo marked an inline comment as done.

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

================
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);
----------------
nlewycky wrote:
> 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.
This only cares about marking global variables **before** evaluating constructors. Whether evaluation fails or not should have no bearing on the fact that a given global variable is only-readable 'writeonce'. Besides, we also do not have to worry about invariant.end here because a global variable never stops being 'writeonce' once marked as such. 

================
Comment at: lib/Transforms/IPO/GlobalOpt.cpp:2881
@@ +2880,3 @@
+  // intrinsic call.
+  processInvariantIntrinsics(InvInfo, F);
+
----------------
nlewycky wrote:
> 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?
Correct. 

================
Comment at: test/Transforms/LoadElim/invariant.ll:39
@@ +38,3 @@
+
+define void @globalex() {
+entry:
----------------
nlewycky wrote:
> 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.
I'm confused. This *is* supposed to test the effect of changes to -globalopt for -gvn...


http://reviews.llvm.org/D15135