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.

Details

Summary

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
lib/Transforms/IPO/GlobalOpt.cpp
2838–2839

I think the usual way to write this is like:

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

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

2848

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

2875–2880

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.

2881

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?

test/Transforms/LoadElim/invariant.ll
39

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.
lib/Transforms/IPO/GlobalOpt.cpp
2848

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

2875–2880

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.

2881

Correct.

test/Transforms/LoadElim/invariant.ll
39

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.