[PATCH] D28077: [PM] Introduce options to enable the (still experimental) new pass manager, and a code path to use it.

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc marked 4 inline comments as done.
chandlerc added a comment.

Thanks for the review, will land shortly with suggested fixes.



================
Comment at: lib/CodeGen/BackendUtil.cpp:757
+  setCommandLineOpts();
+  cl::PrintOptionValues();
+
----------------
mehdi_amini wrote:
> This is different from the legacy path, why?
Stale line. Meant to nuke it, we have it in the correct location below already.


================
Comment at: lib/CodeGen/BackendUtil.cpp:762
+  CreateTargetMachine(/*MustCreateTM*/ true);
+  TheModule->setDataLayout(TM->createDataLayout());
+
----------------
mehdi_amini wrote:
> The legacy path accounts for the possibility of failing to create the TM
> 
> ```
>   CreateTargetMachine(UsesCodeGen);
> 
>   if (UsesCodeGen && !TM)
>     return;
> ```
Good catch, done. I was too focused on the fact that TM was optional before.


================
Comment at: lib/CodeGen/BackendUtil.cpp:831
+  // Now that we have all of the passes ready, run them.
+  MPM.run(*TheModule, MAM);
+
----------------
mehdi_amini wrote:
> No need to wrap in a block with a `PrettyStackTraceString` like the legacy PM?
No, we should wrap it up similarly.


================
Comment at: test/Driver/clang_f_opts.c:475
+// RUN: %clang -### -fexperimental-new-pass-manager -fno-experimental-new-pass-manager %s 2>&1 | FileCheck --check-prefix=CHECK --check-prefix=CHECK-NO-NEW-PM %s
+// CHECK-NOT: argument unused
+// CHECK-NEW-PM: -fexperimental-new-pass-manager
----------------
mehdi_amini wrote:
> Not critical, but using the "CHECK" prefix isn't nice in a file intended to be shared for multiple tests.
Good point!


https://reviews.llvm.org/D28077