This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

Summary

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

Diff Detail

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
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
This revision was automatically updated to reflect the committed changes.
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.