Page MenuHomePhabricator

[lib/LTO] Add a way to run a custom pipeline
ClosedPublic

Authored by davide on Aug 31 2016, 11:25 AM.

Details

Summary

We use this in lld, so it's kinda needed for parity with what's used there.
I might try to add an option for that in the gold-plugin as well.

Diff Detail

Repository
rL LLVM

Event Timeline

davide updated this revision to Diff 69887.Aug 31 2016, 11:25 AM
davide retitled this revision from to [lib/LTO] Add a way to run a custom pipeline.
davide updated this object.
davide added reviewers: rafael, tejohnson.
davide added a subscriber: llvm-commits.

Phab doesn't seem to send e-mails today.

mehdi_amini added inline comments.Aug 31 2016, 11:36 AM
lib/LTO/LTOBackend.cpp
145 ↗(On Diff #69887)

Please use better variable name.

(I know the rest of the file doesn't, but we should fix it and not spread this).

rafael edited edge metadata.Aug 31 2016, 11:53 AM

Can you write a test with llvm-lto2?

mehdi_amini added inline comments.Aug 31 2016, 11:59 AM
lib/LTO/LTOBackend.cpp
145 ↗(On Diff #69887)

Instead of taking a config, you can pass directly the string and the bool you need.
Then, it looks like this function is something that will be needed in multiple place in LLVM and could be implemented somewhere else.

165 ↗(On Diff #69887)

We always verify the input.

davide updated this revision to Diff 70478.Sep 6 2016, 3:28 PM
davide edited edge metadata.

Sorry, I was distracted by other things. This should address part of the comments and provide feedback where I have a different opinion. Thanks!

mehdi_amini added inline comments.Sep 6 2016, 3:32 PM
lib/LTO/LTOBackend.cpp
125 ↗(On Diff #70478)

It seems like this should be provided in LLVM somewhere, why would this be different between opt or clang for instance and LTO?

davide added inline comments.Sep 6 2016, 3:36 PM
lib/LTO/LTOBackend.cpp
145 ↗(On Diff #69887)

I changed the arguments to the function. About centralizing somewhere, maybe.
This can probably be shared with opt but I'd like to discuss this with Chandler before exposing a new API. So I'll leave the static helper here for now.

145 ↗(On Diff #69887)

I'd rather be consistent with what's there already, and change the whole naming convention in a second pass, if you don't mind.

165 ↗(On Diff #69887)

Not in lld, but, OK, I think it's a decent idea, so I made the change.

mehdi_amini added inline comments.Sep 6 2016, 3:40 PM
lib/LTO/LTOBackend.cpp
180 ↗(On Diff #70478)

About centralizing somewhere, maybe.
This can probably be shared with opt but I'd like to discuss this with Chandler before exposing a new API. So I'll leave the static helper here for now.

Sorry, it does not make sense to me, opt has to need the same thing.
(indeed I just checked and it is calling llvm::runPassPipeline. Why aren't you using it?)

I'd rather be consistent with what's there already, and change the whole naming convention in a second pass, if you don't mind.

If you don't want to break consistency, that's fine: first change the names in the whole file, and then update this patch.

mehdi_amini accepted this revision.Sep 6 2016, 7:08 PM
mehdi_amini added a reviewer: mehdi_amini.

LGTM

This revision is now accepted and ready to land.Sep 6 2016, 7:08 PM
This revision was automatically updated to reflect the committed changes.