This is an archive of the discontinued LLVM Phabricator instance.

Port demanded-bits to the new pass manager
ClosedPublic

Authored by mkuper on Mar 31 2016, 5:22 PM.

Details

Summary

My first stab at this, so I may have gotten it horribly wrong, especially since this is a lazy pass.
I've preserved the way in which it is currently lazy, but any better suggestions are welcome.

Also, I'm not sure about the Optional<> result. And there's the somewhat ugly mutable member, but that's just instead of having the const_cast in the previous version. James, I think you introduced that, if you prefer to go back to the const_cast, I don't mind.

(I wanted to try to port something simple just to see how this works, and BDCE looked liked it ought to be simple enough - but it relies on DemandedBits, so...)

Diff Detail

Repository
rL LLVM

Event Timeline

mkuper updated this revision to Diff 52314.Mar 31 2016, 5:22 PM
mkuper retitled this revision from to Port demanded-bits to the new pass manager.
mkuper updated this object.
mkuper added reviewers: mehdi_amini, jmolloy, hfinkel.
mkuper added subscribers: llvm-commits, chandlerc.
jmolloy edited edge metadata.Apr 13 2016, 7:41 AM
jmolloy added a subscriber: jmolloy.

Hi,

Apologies, I am away on vacation for the next week. I'll be unable to review this until at least the 23rd April.

Cheers,

James

chandlerc requested changes to this revision.Apr 18 2016, 12:21 AM
chandlerc edited edge metadata.
chandlerc added inline comments.
include/llvm/Analysis/DemandedBits.h
39 ↗(On Diff #52314)

I would just call this "DemandedBits". In many cases, the original name was already a useful noun describing the results and query API and not really anything to do with the analysis.

71 ↗(On Diff #52314)

I've been calling these "...WrapperPass" to signify that they actually contain the result object for the legacy PM.

DomTree and LoopInfo I think are reasonable examples here.

73 ↗(On Diff #52314)

Why mutable?

This revision now requires changes to proceed.Apr 18 2016, 12:21 AM

Thanks, Chandler.

include/llvm/Analysis/DemandedBits.h
39 ↗(On Diff #52314)

Right, I just thought it may be confusing to reuse the old name of the pass for the result.
Will change.

71 ↗(On Diff #52314)

Ack.

73 ↗(On Diff #52314)

Because the pass is lazy, print() has to run performAnalysis().
Since print() is an overloaded const method, the old version had:

// This is gross. But the alternative is making all the state mutable
// just because of this one debugging method.
const_cast<DemandedBits*>(this)->performAnalysis();

I thought having the result mutable was nicer, but I'm ok with keeping the const_cast if you prefer it.
Or anything else which is nicer than either.

mkuper updated this revision to Diff 54108.Apr 18 2016, 1:35 PM
mkuper edited edge metadata.
mkuper removed a subscriber: chandlerc.

Changed names per chandler's comments.

chandlerc accepted this revision.Apr 18 2016, 2:27 PM
chandlerc edited edge metadata.

LGTM, thanks!!

include/llvm/Analysis/DemandedBits.h
73 ↗(On Diff #54108)

Wow. Yea, I'm fine with the mutable member, that makes perfect sense.

I think the "right" way to fix this is much the way the new pass manager does it -- have separate printing passes from analysis passes so that the printing passes can actually walk the lazy data structures.

Anyways, no need to try to clean this up for the old pass manager, this seems like a strict improvement over the prior state of the world.

This revision is now accepted and ready to land.Apr 18 2016, 2:27 PM
This revision was automatically updated to reflect the committed changes.