Page MenuHomePhabricator

Extend the use of @llvm.invariant.start/end intrinsics for GVN-based load elimination purposes to also handle global variables.
Needs ReviewPublic

Authored by lvoufo on Dec 1 2015, 5:00 PM.



The -globalopt pass is made to require -invariant-info-marker, similarly to -gvn. (Note: Both -gvn and -globalopt require basic AA.)

The test case is also expanded to illustrate the subtle nuances and expected similarities between local and global variables.

Diff Detail

Event Timeline

lvoufo updated this revision to Diff 41580.Dec 1 2015, 5:00 PM
lvoufo retitled this revision from to Extend the use of @llvm.invariant.start/end intrinsics for GVN-based load elimination purposes to also handle global variables..
lvoufo updated this object.
lvoufo added reviewers: reames, nlewycky, chandlerc.
lvoufo added a subscriber: llvm-commits.
lvoufo updated this revision to Diff 41586.Dec 1 2015, 6:24 PM

Fix typo in patch.

lvoufo updated this revision to Diff 42220.Dec 8 2015, 2:05 PM
nlewycky added inline comments.Dec 8 2015, 9:44 PM

I think the usual way to write this is like:

CallSite CS(&*CurInst);
if (CS) {

DbgInfoIntrinsics are Intrinsics, you don't need to test for them separately. Also remove "..." because there aren't any more things being checked here.


Isn't this a double-increment? You'll skip CurInst here, then the for loop increment will skip whatever instruction comes after?


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.


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?


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.

lvoufo marked an inline comment as done.Dec 9 2015, 9:21 AM
lvoufo added inline comments.

Yes, it is. Oops. Thanks for the catch!


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.




I'm confused. This *is* supposed to test the effect of changes to -globalopt for -gvn...

lvoufo marked an inline comment as done.Dec 9 2015, 9:43 AM

To summarize my thoughts after these comments, this is an analysis that should be kept outside of -globalopt, since it does not affect the logic of -globalopt at all... So, I'm working on that.

reames resigned from this revision.Apr 18 2016, 4:56 PM
reames removed a reviewer: reames.