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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
