This is an archive of the discontinued LLVM Phabricator instance.

[PM] Port PartialInlining to new PM
ClosedPublic

Authored by eraman on Jun 24 2016, 2:15 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

eraman updated this revision to Diff 61836.Jun 24 2016, 2:15 PM
eraman retitled this revision from to [PM] Port PartialInlining to new PM.
eraman updated this object.
eraman added a reviewer: davide.
eraman added a subscriber: llvm-commits.
davide accepted this revision.Jun 24 2016, 2:59 PM
davide edited edge metadata.

This patch is nice. Just one minor comment. After that, LGTM.

lib/Transforms/IPO/PartialInlining.cpp
33 ↗(On Diff #61836)

This one can be removed, no?

This revision is now accepted and ready to land.Jun 24 2016, 2:59 PM

There's always the tradeoff between:

  • using an Impl object as you did
  • staticizing the member variables in order to share code them between old and new PM.

I'd prefer 2) in this case, because there's only one member variable, but I have no strong opinions about that.
With that in mind, I don't think it's worth rewriting this patch to use the other approach, as the boilerplate introduced is very little.

eraman updated this revision to Diff 61861.Jun 24 2016, 5:16 PM
eraman edited edge metadata.

Address Davide's comments

eraman marked an inline comment as done.Jun 24 2016, 5:17 PM

There's always the tradeoff between:

  • using an Impl object as you did
  • staticizing the member variables in order to share code them between old and new PM.

I'd prefer 2) in this case, because there's only one member variable, but I have no strong opinions about that.
With that in mind, I don't think it's worth rewriting this patch to use the other approach, as the boilerplate introduced is very little.

I don't have a strong opinion. As you say, there is very little bolierplate and I'll keep this version.

This revision was automatically updated to reflect the committed changes.