This is an archive of the discontinued LLVM Phabricator instance.

[PM] Port MergedLoadStoreMotion to the new pass manager, take two.
ClosedPublic

Authored by davide on Jun 16 2016, 5:49 PM.

Details

Summary

This is a redacted version of my original pass, modeled after Daniel Berlin's comments.
Please take a closer look.

Diff Detail

Event Timeline

davide updated this revision to Diff 61052.Jun 16 2016, 5:49 PM
davide retitled this revision from to [PM] Port MergedLoadStoreMotion to the new pass manager, take two..
davide updated this object.
davide added reviewers: dberlin, davidxl.
davide added a subscriber: llvm-commits.
davidxl added inline comments.Jun 16 2016, 9:38 PM
include/llvm/Transforms/Scalar/MergedLoadStoreMotion.h
96

Any reason we need to make it a member field, instead of just being created as a local instance at the time it is run? With that there is no need to hoist the class into the header.

silvas added a subscriber: silvas.Jun 16 2016, 10:41 PM

A couple comments.

include/llvm/Transforms/Scalar/MergedLoadStoreMotion.h
96

In this case it does not matter. In other cases, it is preferable to .clear() maps/vectors between calls to run instead of reallocating from scratch (to save malloc traffic). E.g. SLPVectorizer in r272766.

I have not done measurement to see if this optimization helps, but it is the existing state of things.

lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
573

Use the new-PM class as the Impl (or just make the Impl local to runOnFunction). See e.g. r272757

davide updated this revision to Diff 61112.Jun 17 2016, 10:51 AM

Make Impl local to runOnFunction()/run() as suggested by David/Sean.
I was worried about an increased malloc() activity so I measured but I wasn't able to discover any substantial difference.
We can re-evaluate in the future, if needed.
Also, Daniel needs to sign-off this patch before it can go in.

dberlin accepted this revision.Jun 17 2016, 12:11 PM
dberlin edited edge metadata.

LGTM.
I really appreciate you taking the time to sit down and do this, i know it's quite annoying when folks ask you to take a different approach after you already got it done once.

This revision is now accepted and ready to land.Jun 17 2016, 12:11 PM

LGTM.
I really appreciate you taking the time to sit down and do this, i know it's quite annoying when folks ask you to take a different approach after you already got it done once.

No problem. Thanks for your time reviewing and helping making the code better. =)

This revision was automatically updated to reflect the committed changes.