[PATCH] D82922: [NewPM] Add explicit init value to -enable-new-pm

Hans Wennborg via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 1 11:56:11 PDT 2020


hans added inline comments.


================
Comment at: llvm/tools/opt/opt.cpp:75
+static cl::opt<bool> EnableNewPassManager(
+    "enable-new-pm", cl::desc("Enable the new pass manager"), cl::init(false));
 
----------------
aeubanks wrote:
> MaskRay wrote:
> > aeubanks wrote:
> > > MaskRay wrote:
> > > > aeubanks wrote:
> > > > > MaskRay wrote:
> > > > > > false is the default value so this appears to be NFC...
> > > > > 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.
> > > > Adding/removing `cl::init(true)` is very straightforward. Why is this NFC change needed?
> > > 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.
> > 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.
> 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.
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.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82922/new/

https://reviews.llvm.org/D82922





More information about the llvm-commits mailing list