This is an archive of the discontinued LLVM Phabricator instance.

Differential D82922

[NewPM] Add explicit init value to -enable-new-pm
ClosedPublic

Authored by aeubanks on Jun 30 2020, 4:26 PM.

Details

Reviewers
ychen
hans
Commits
rG9a5e3a439232: [NewPM] Add explicit init value to -enable-new-pm
Summary

So it's easier to test with it on by default.

Diff Detail

Unit TestsFailed

TimeTest
10,580 mslinux > libomp.env::Unknown Unit Message ("")
1,520 mslinux > libomp.worksharing/for::Unknown Unit Message ("")

Event Timeline

aeubanks created this revision.Jun 30 2020, 4:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2020, 4:26 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
ychen accepted this revision.Jun 30 2020, 5:40 PM

LGTM

This revision is now accepted and ready to land.Jun 30 2020, 5:40 PM
Harbormaster failed remote builds in B62423: Diff 274645!Jun 30 2020, 5:56 PM
Closed by commit rG9a5e3a439232: [NewPM] Add explicit init value to -enable-new-pm (authored by aeubanks). · Explain WhyJun 30 2020, 7:00 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added a subscriber: MaskRay.Jun 30 2020, 9:49 PM
MaskRay added inline comments.
llvm/tools/opt/opt.cpp
75

false is the default value so this appears to be NFC...

aeubanks marked an inline comment as done.Jun 30 2020, 10:37 PM
aeubanks added inline comments.
llvm/tools/opt/opt.cpp
75

I can more easily change the default value to true and test this way.
If there is a better way I'd like to know.

MaskRay added inline comments.Jun 30 2020, 10:51 PM
llvm/tools/opt/opt.cpp
75

Adding/removing cl::init(true) is very straightforward. Why is this NFC change needed?

aeubanks marked an inline comment as done.Jul 1 2020, 9:02 AM
aeubanks added inline comments.
llvm/tools/opt/opt.cpp
75

It's easier for me to replace "false" with "true" than to type the whole thing out.
Also, at some point I want to add a CMake variable which controls this flag's default value (same as the one that controls clang's NPM default behavior), and this is one step to that.

MaskRay added inline comments.Jul 1 2020, 11:09 AM
llvm/tools/opt/opt.cpp
75

If adding a cmake variable is like moving from 0 to 1, then this NFC patch seems to me 0 to 0.1

I wonder why we didn't move directly to 1. This patch added unnecessary churn to the history.

aeubanks marked an inline comment as done.Jul 1 2020, 11:51 AM
aeubanks added inline comments.
llvm/tools/opt/opt.cpp
75

Ok, maybe the CMake argument isn't so strong, but this patch still does make my life just a little easier when testing NPM with check-llvm. I'm not sure where the threshold for a patch's usefulness is for it to be worth a commit.

hans added inline comments.Jul 1 2020, 11:55 AM
llvm/tools/opt/opt.cpp
75

I don't think churning the version history is a concern here, and I don't see that the patch is doing anything weird here, just spelling out the default value and making it easier to flip with a one-liner.

If this makes Arthur's work towards enabling the new pm easier, I believe that certainly puts it above the threshold.

Revision Contents

PathSize
llvm/
tools/
opt/
5 lines

Diff 274645

llvm/tools/opt/opt.cpp

Show First 20 Lines • Show All 65 Lines • ▼ Show 20 Lines
static codegen::RegisterCodeGenFlags CFG; static codegen::RegisterCodeGenFlags CFG;
// The OptimizationList is automatically populated with registered Passes by the // The OptimizationList is automatically populated with registered Passes by the
// PassNameParser. // PassNameParser.
// //
static cl::list<const PassInfo*, bool, PassNameParser> static cl::list<const PassInfo*, bool, PassNameParser>
PassList(cl::desc("Optimizations available:")); PassList(cl::desc("Optimizations available:"));
static cl::opt<bool> static cl::opt<bool> EnableNewPassManager(
EnableNewPassManager("enable-new-pm", "enable-new-pm", cl::desc("Enable the new pass manager"), cl::init(false));
MaskRayUnsubmitted
Not Done
ReplyInline Actions

false is the default value so this appears to be NFC...

MaskRay: false is the default value so this appears to be NFC...
aeubanksAuthorUnsubmitted
Done
ReplyInline Actions

I can more easily change the default value to true and test this way.
If there is a better way I'd like to know.

aeubanks: I can more easily change the default value to true and test this way. If there is a better way…
MaskRayUnsubmitted
Not Done
ReplyInline Actions

Adding/removing cl::init(true) is very straightforward. Why is this NFC change needed?

MaskRay: Adding/removing `cl::init(true)` is very straightforward. Why is this NFC change needed?
aeubanksAuthorUnsubmitted
Done
ReplyInline Actions

It's easier for me to replace "false" with "true" than to type the whole thing out.
Also, at some point I want to add a CMake variable which controls this flag's default value (same as the one that controls clang's NPM default behavior), and this is one step to that.

aeubanks: It's easier for me to replace "false" with "true" than to type the whole thing out. Also, at…
MaskRayUnsubmitted
Not Done
ReplyInline Actions

If adding a cmake variable is like moving from 0 to 1, then this NFC patch seems to me 0 to 0.1

I wonder why we didn't move directly to 1. This patch added unnecessary churn to the history.

MaskRay: If adding a cmake variable is like moving from 0 to 1, then this NFC patch seems to me 0 to 0.1…
aeubanksAuthorUnsubmitted
Done
ReplyInline Actions

Ok, maybe the CMake argument isn't so strong, but this patch still does make my life just a little easier when testing NPM with check-llvm. I'm not sure where the threshold for a patch's usefulness is for it to be worth a commit.

aeubanks: Ok, maybe the CMake argument isn't so strong, but this patch still does make my life just a…
hansUnsubmitted
Not Done
ReplyInline Actions

I don't think churning the version history is a concern here, and I don't see that the patch is doing anything weird here, just spelling out the default value and making it easier to flip with a one-liner.

If this makes Arthur's work towards enabling the new pm easier, I believe that certainly puts it above the threshold.

hans: I don't think churning the version history is a concern here, and I don't see that the patch is…
cl::desc("Enable the new pass manager"));
// This flag specifies a textual description of the optimization pass pipeline // This flag specifies a textual description of the optimization pass pipeline
// to run over the module. This flag switches opt to use the new pass manager // to run over the module. This flag switches opt to use the new pass manager
// infrastructure, completely disabling all of the flags specific to the old // infrastructure, completely disabling all of the flags specific to the old
// pass management. // pass management.
static cl::opt<std::string> PassPipeline( static cl::opt<std::string> PassPipeline(
"passes", "passes",
cl::desc("A textual description of the pass pipeline for optimizing"), cl::desc("A textual description of the pass pipeline for optimizing"),
▲ Show 20 Lines • Show All 955 Lines • Show Last 20 Lines