[PATCH] D33464: [PM][WIP] Enable out-of-tree registration of passes with the new PassBuilder

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Pretty desperately need to factor the mocking helpers out, but that's not your fault. The actual test code is really clean IMO.

As discussed in IRC, I'd still like to remove the synthesis of inner IR-unit pipelines for EPs and remove the extra parsing APIs. I don't think they pull their weight. But the opt check is fine with a tweak below.

More detailed comments on the unittest mostly.



================
Comment at: include/llvm/Passes/PassBuilder.h:472
+  /// {{@ Register pipeline parsing callbacks with this pass builder instance.
+  /// Using these callbacks, calles can parse both a single pass name, as well
+  /// as entire sub-pipelines, and populate the PassManager instance
----------------
`calles` -> `callers`


================
Comment at: test/Other/new-pm-defaults.ll:29-33
+; RUN: opt -disable-verify -debug-pass-manager \
+; RUN:     -passes-ep-module-optimizer-early='no-op-module' \
+; RUN:     -passes='default<O3>' -S  %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=CHECK-O --check-prefix=CHECK-O3 \
+; RUN:     --check-prefix=CHECK-EP-ModuleEarly
----------------
chandlerc wrote:
> What about having just one `opt` invocation with a no-op pass in *every* extension point? I don't think you lose any coverage, and then you have a much easier check prefix of just `CHECK-EP`.
I see, you want to check the *different* extension points.

OK, in this case, I would still name the check prefixes using ALL-CAPS-WITH-DASHES rather than the mixed case you have.


================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:159
+
+/// {{@ Mock handles for passes for the IRUnits Module, CGSCC, Function, Loop.
+/// These handles define the appropriate run() mock interface for the respective
----------------
API grouping doxygen doesn't seem terribly useful inside the bowels of the test.


================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:270
+/// the given PassManagerT.
+template <typename _IRUnitT, typename... ExtraPassArgTs,
+          typename... ExtraAnalysisArgTs>
----------------
You can't name identifiers with leading `_` and a capital letter.

Perhaps `TestIRUnitT`?


================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:295-300
+  bool run(StringRef PipelineText) {
+    if (!PB.parsePassPipeline(this->PM, PipelineText, true))
+      return false;
+    PM.run(*M, AM);
+    return true;
+  }
----------------
I would inline this into the caller so you can write more precise expectations.

For example:

  ASSERT_TRUE(PB.parsePassPipeline(this->PM, PipelineText, true)) << PipelineText;

Seems likely to give a much nicer error when it fails. =]

I guess you could provide wrappers around `PB.parsePassPipeline` to ease some of the code, but it's not clear it is worth it. The test should be easy to inspect even if a very minor of repetition is involved.

Also, do you need the `this->` here?


================
Comment at: unittests/IR/PassBuilderCallbacksTest.cpp:311-312
+      : M(parseIR(Context,
+                  "declare void @bar() local_unnamed_addr\n"
+                  "define void @foo(i32 %n) local_unnamed_addr {\n"
+                  "entry:\n"
----------------
Why `local_unnamed_addr`? Seems like this test should really be as simple as possible?


https://reviews.llvm.org/D33464