This is part of effort in removing -enable-new-pm flag.
As a prat of this effort one of example passes SimplifyCFG must
be ported to new PM which will allow to remove the flag
calls from the tests that are using this pass.
Details
Details
- Reviewers
aeubanks - Commits
- rGd291f1fd0945: [LegacyPM] Port example pass SimplifyCFG to new PM
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
This is currently WIP patch, because I have some issues in understanding how exactly the porting should happen. I've been following @aeubanks recommendation from discourse, but two main things that I found unclear so far are:
- Should I retain legacy code, or can I safely drop it? In Bye pass I can see that legacy pass is still present, which for me makes little sense to copy for this pass. Especially because goal of this effort is to drop flag enabling run on legacy PM.
- Looking for example at llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp it seems that different approach has been used to add support for new PM, which is closer to what documentation is showing, rather what is presented in Bye pass. Mainly in the are of registration of new pass as well as pass options, which are using special processing function as I understand. Which way is "the proper one"?
| llvm/examples/IRTransforms/SimplifyCFG.h | ||
|---|---|---|
| 19 | I couldn't find call to createSimplifyCFGPass anywhere. In other places (e.g. llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp) similar function is actually used // Public interface to the CFGSimplification pass
FunctionPass *
llvm::createCFGSimplificationPass(SimplifyCFGOptions Options,
std::function<bool(const Function &)> Ftor) {
return new CFGSimplifyPass(Options, std::move(Ftor));
}Do we really need this function here? | |
Comment Actions
Yes we should drop everything specific to the legacy pass manager here.
- Looking for example at llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp it seems that different approach has been used to add support for new PM, which is closer to what documentation is showing, rather what is presented in Bye pass. Mainly in the are of registration of new pass as well as pass options, which are using special processing function as I understand. Which way is "the proper one"?
Since this is a plugin rather than a real in-tree pass that's always available, it doesn't use the same registration. https://reviews.llvm.org/D136626 updates new PM plugin docs.
I wouldn't worry about pass options, using a cl::opt is fine for now. We can make the pass option changes later if we want.
| llvm/examples/IRTransforms/SimplifyCFG.cpp | ||
|---|---|---|
| 413 | a little misleading, I'd say MadeChange instead | |
| 419 | formatting | |
| 433 | this pass definitely doesn't preserve the CFG, we can just return PreservedAnalyses::none() | |
| 443 | I don't think the legacy pass does this, drop this part? | |
| 469 | getSimplifyCFGPluginInfo? | |
| llvm/examples/IRTransforms/SimplifyCFG.h | ||
| 19 | it was probably just an example on how you'd typically create a legacy pass. yes we should remove it and the InitializePasses.cpp/h in this folder | |
Comment Actions
Addressed (hopefully) all review comments.
I tried to understand how the naming schemas should work for registering new pass plugin, but there is a chance I might have missed something or misunderstood.
Bottom-line question is, what should I change, so I can call this pass with flags -tut-simplifycfg -tut-simplifycfg-version=XX?
My best guess is that I might be missing one extra registration step. I added -DLLVM_INCLUDE_EXAMPLES=ON to cmake which resulted in -- Registering SimplifyCFG as a pass plugin (static build: OFF) which indicates that I should somehow register plugin with PassBuilder as dynamically linked. As described here:
Load plugin dynamically.
auto Plugin = PassPlugin::Load(PathToPlugin);
if (!Plugin)
report_error();
Register plugin extensions in PassBuilder.
Plugin.registerPassBuilderCallbacks(PB);
But from documentation and look at sources it is unclear how/where exactly it should be done. I'll appreciate any pointers.
Comment Actions
-passes=tut-simplifycfg will call into the lambda passed to registerPipelineParsingCallback. -tut-simplifycfg-version=XX should still be the same
My best guess is that I might be missing one extra registration step. I added -DLLVM_INCLUDE_EXAMPLES=ON to cmake which resulted in -- Registering SimplifyCFG as a pass plugin (static build: OFF) which indicates that I should somehow register plugin with PassBuilder as dynamically linked. As described here:
Load plugin dynamically.
auto Plugin = PassPlugin::Load(PathToPlugin);
if (!Plugin)
report_error();
Register plugin extensions in PassBuilder.
Plugin.registerPassBuilderCallbacks(PB);But from documentation and look at sources it is unclear how/where exactly it should be done. I'll appreciate any pointers.
-DLLVM_INCLUDE_EXAMPLES=ON is the default, you actually want -DLLVM_BUILD_EXAMPLES=ON (it's very confusing but the tests wouldn't run with LLVM_INCLUDE_EXAMPLES, only with LLVM_BUILD_EXAMPLES (found by following through config.build_examples in llvm/test/Examples/lit.local.cfg)
that code is already done in opt in NewPMDriver.cpp which is aware of example in-tree plugins; the docs were meant to be for if you're writing your own tool:
// For any loaded plugins, let them register pass builder callbacks. for (auto &PassPlugin : PassPlugins) PassPlugin.registerPassBuilderCallbacks(PB);
diff --git a/llvm/examples/IRTransforms/CMakeLists.txt b/llvm/examples/IRTransforms/CMakeLists.txt index 45bc20bdc022..9ad056048090 100644 --- a/llvm/examples/IRTransforms/CMakeLists.txt +++ b/llvm/examples/IRTransforms/CMakeLists.txt @@ -1,19 +1,12 @@ -if(LLVM_SIMPLIFYCFG_LINK_INTO_TOOLS) - message(WARNING "Setting LLVM_SIMPLIFYCFG_LINK_INTO_TOOLS=ON only makes sense for testing purpose") +if(LLVM_EXAMPLEIRTRANSFORMS_LINK_INTO_TOOLS) + message(WARNING "Setting LLVM_EXAMPLEIRTRANSFORMS_LINK_INTO_TOOLS=ON only makes sense for testing purpose") endif() - -set(LLVM_LINK_COMPONENTS - Analysis - Core - Support - ) - -add_llvm_pass_plugin(SimplifyCFG +add_llvm_pass_plugin(ExampleIRTransforms SimplifyCFG.cpp - ADDITIONAL_HEADER_DIRS DEPENDS intrinsics_gen + BUILDTREE_ONLY ) install(TARGETS ${name} RUNTIME DESTINATION "${LLVM_EXAMPLES_INSTALL_DIR}") diff --git a/llvm/examples/IRTransforms/SimplifyCFG.cpp b/llvm/examples/IRTransforms/SimplifyCFG.cpp index 9aafa5839120..29e705d11643 100644 --- a/llvm/examples/IRTransforms/SimplifyCFG.cpp +++ b/llvm/examples/IRTransforms/SimplifyCFG.cpp @@ -395,7 +395,7 @@ struct SimplifyCFGPass : public PassInfoMixin<SimplifyCFGPass> { } // namespace /* New PM Registration */ -llvm::PassPluginLibraryInfo getSimplifyCFGPluginInfo() { +llvm::PassPluginLibraryInfo getExampleIRTransformsPluginInfo() { return {LLVM_PLUGIN_API_VERSION, "SimplifyCFG", LLVM_VERSION_STRING, [](PassBuilder &PB) { PB.registerVectorizerStartEPCallback( @@ -417,6 +417,6 @@ llvm::PassPluginLibraryInfo getSimplifyCFGPluginInfo() { #ifndef LLVM_SIMPLIFYCFG_LINK_INTO_TOOLS extern "C" LLVM_ATTRIBUTE_WEAK ::llvm::PassPluginLibraryInfo llvmGetPassPluginInfo() { - return getSimplifyCFGPluginInfo(); + return getExampleIRTransformsPluginInfo(); } #endif diff --git a/llvm/test/CMakeLists.txt b/llvm/test/CMakeLists.txt index 01e2a6ebf6c7..b42aa2d014b8 100644 --- a/llvm/test/CMakeLists.txt +++ b/llvm/test/CMakeLists.txt @@ -16,6 +16,7 @@ llvm_canonicalize_cmake_booleans( LLVM_BUILD_EXAMPLES LLVM_ENABLE_PLUGINS LLVM_BYE_LINK_INTO_TOOLS + LLVM_EXAMPLEIRTRANSFORMS_LINK_INTO_TOOLS LLVM_HAVE_TF_AOT LLVM_HAVE_TF_API LLVM_INLINER_MODEL_AUTOGENERATED diff --git a/llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg1.ll b/llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg1.ll index 20f20ea5a102..35dac1dba927 100644 --- a/llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg1.ll +++ b/llvm/test/Examples/IRTransforms/SimplifyCFG/tut-simplify-cfg1.ll @@ -1,7 +1,7 @@ ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py -; RUN: opt -tut-simplifycfg -tut-simplifycfg-version=v1 -enable-new-pm=0 -S < %s | FileCheck %s -; RUN: opt -tut-simplifycfg -tut-simplifycfg-version=v2 -enable-new-pm=0 -S < %s | FileCheck %s -; RUN: opt -tut-simplifycfg -tut-simplifycfg-version=v3 -enable-new-pm=0 -S < %s | FileCheck %s +; RUN: opt %loadexampleirtransforms -passes=tut-simplifycfg -tut-simplifycfg-version=v1 -S < %s | FileCheck %s +; RUN: opt %loadexampleirtransforms -passes=tut-simplifycfg -tut-simplifycfg-version=v2 -S < %s | FileCheck %s +; RUN: opt %loadexampleirtransforms -passes=tut-simplifycfg -tut-simplifycfg-version=v3 -S < %s | FileCheck %s define i32 @simp1() { ; CHECK-LABEL: @simp1( diff --git a/llvm/test/lit.cfg.py b/llvm/test/lit.cfg.py index 500cbd68b494..12d9801050e3 100644 diff --git a/llvm/test/lit.cfg.py b/llvm/test/lit.cfg.py index 500cbd68b494..12d9801050e3 100644 --- a/llvm/test/lit.cfg.py +++ b/llvm/test/lit.cfg.py @@ -288,6 +288,13 @@ else: .format(config.llvm_shlib_dir, config.llvm_shlib_ext))) +if config.linked_exampleirtransforms_extension: + config.substitutions.append(('%loadexampleirtransforms','')) +else: + config.substitutions.append(('%loadexampleirtransforms', + '-load-pass-plugin={}/ExampleIRTransforms{}' + .format(config.llvm_shlib_dir, + config.llvm_shlib_ext))) # Static libraries are not built if BUILD_SHARED_LIBS is ON. if not config.build_shared_libs and not config.link_llvm_dylib: diff --git a/llvm/test/lit.site.cfg.py.in b/llvm/test/lit.site.cfg.py.in index 5531732e16bc..bc27c04bc9db 100644 --- a/llvm/test/lit.site.cfg.py.in +++ b/llvm/test/lit.site.cfg.py.in @@ -53,6 +53,7 @@ config.have_opt_viewer_modules = @LLVM_HAVE_OPT_VIEWER_MODULES@ config.libcxx_used = @LLVM_LIBCXX_USED@ config.has_plugins = @LLVM_ENABLE_PLUGINS@ config.linked_bye_extension = @LLVM_BYE_LINK_INTO_TOOLS@ +config.linked_exampleirtransforms_extension = @LLVM_EXAMPLEIRTRANSFORMS_LINK_INTO_TOOLS@ config.have_tf_aot = @LLVM_HAVE_TF_AOT@ config.have_tf_api = @LLVM_HAVE_TF_API@ config.llvm_inliner_model_autogenerated = @LLVM_INLINER_MODEL_AUTOGENERATED@ diff --git a/llvm/tools/opt/CMakeLists.txt b/llvm/tools/opt/CMakeLists.txt index e5d9d28d5006..5d2d4cd5196e 100644 --- a/llvm/tools/opt/CMakeLists.txt +++ b/llvm/tools/opt/CMakeLists.txt @@ -38,7 +38,3 @@ add_llvm_tool(opt SUPPORT_PLUGINS ) export_executable_symbols_for_plugins(opt) - -if(LLVM_BUILD_EXAMPLES) - target_link_libraries(opt PRIVATE ExampleIRTransforms) -endif(LLVM_BUILD_EXAMPLES) diff --git a/llvm/test/lit.site.cfg.py.in b/llvm/test/lit.site.cfg.py.in index 5531732e16bc..bc27c04bc9db 100644 --- a/llvm/test/lit.site.cfg.py.in +++ b/llvm/test/lit.site.cfg.py.in @@ -53,6 +53,7 @@ config.have_opt_viewer_modules = @LLVM_HAVE_OPT_VIEWER_MODULES@ config.libcxx_used = @LLVM_LIBCXX_USED@ config.has_plugins = @LLVM_ENABLE_PLUGINS@ config.linked_bye_extension = @LLVM_BYE_LINK_INTO_TOOLS@ +config.linked_exampleirtransforms_extension = @LLVM_EXAMPLEIRTRANSFORMS_LINK_INTO_TOOLS@ config.have_tf_aot = @LLVM_HAVE_TF_AOT@ config.have_tf_api = @LLVM_HAVE_TF_API@ config.llvm_inliner_model_autogenerated = @LLVM_INLINER_MODEL_AUTOGENERATED@ diff --git a/llvm/tools/opt/CMakeLists.txt b/llvm/tools/opt/CMakeLists.txt index e5d9d28d5006..5d2d4cd5196e 100644 --- a/llvm/tools/opt/CMakeLists.txt +++ b/llvm/tools/opt/CMakeLists.txt @@ -38,7 +38,3 @@ add_llvm_tool(opt SUPPORT_PLUGINS ) export_executable_symbols_for_plugins(opt) - -if(LLVM_BUILD_EXAMPLES) - target_link_libraries(opt PRIVATE ExampleIRTransforms) -endif(LLVM_BUILD_EXAMPLES) diff --git a/llvm/tools/opt/opt.cpp b/llvm/tools/opt/opt.cpp index 8dac02a356b1..123a7209c482 100644 --- a/llvm/tools/opt/opt.cpp +++ b/llvm/tools/opt/opt.cpp @@ -392,10 +392,6 @@ static TargetMachine* GetTargetMachine(Triple TheTriple, StringRef CPUStr, codegen::getExplicitCodeModel(), GetCodeGenOptLevel()); } -#ifdef BUILD_EXAMPLES -void initializeExampleIRTransforms(llvm::PassRegistry &Registry); -#endif - struct TimeTracerRAII { TimeTracerRAII(StringRef ProgramName) { if (TimeTrace) @@ -531,10 +527,6 @@ int main(int argc, char **argv) { initializeReplaceWithVeclibLegacyPass(Registry); initializeJMCInstrumenterPass(Registry); -#ifdef BUILD_EXAMPLES - initializeExampleIRTransforms(Registry); -#endif - SmallVector<PassPlugin, 1> PluginList; PassPlugins.setCallback([&](const std::string &PluginPath) { auto Plugin = PassPlugin::Load(PluginPath);
I tried matching Bye's CMakeLists.txt as much as possible, then also copied the lit substitutions to specify the .so file when the plugin is dynamically linked. The rest of the changes fix build errors with and without -DLLVM_EXAMPLEIRTRANSFORMS_LINK_INTO_TOOLS=ON.
| llvm/examples/IRTransforms/SimplifyCFG.cpp | ||
|---|---|---|
| 43 | should be able to remove this and Pass.h | |
| 396 | still wrong indentation | |
| 402–403 | warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
default:should just delete this | |
| 443 | registerVectorizerStartEPCallback part should still be removed | |
Comment Actions
I still need to do some more debugging because with current patch following tests started to fail:
Failed Tests (5): LLVM :: Examples/Kaleidoscope/Chapter4.test LLVM :: Examples/Kaleidoscope/Chapter5.test LLVM :: Examples/Kaleidoscope/Chapter6.test LLVM :: Examples/Kaleidoscope/Chapter7.test LLVM :: Feature/load_extension.ll
My guess is that changes in either llvm/tools/opt/CMakeLists.txt or llvm/tools/opt/opt.cpp but I need to confirm that.
Comment Actions
the Kaleidoscope tests pass for me locally
I did have to add this diff
--- a/llvm/test/CMakeLists.txt +++ b/llvm/test/CMakeLists.txt @@ -190,6 +190,7 @@ if(LLVM_BUILD_EXAMPLES) if (NOT WIN32) list(APPEND LLVM_TEST_DEPENDS Bye + ExampleIRTransforms ) endif() endif()
I'd probably add the if (NOT WIN32) from llvm/examples/Bye/CMakeLists.txt to llvm/examples/IRTransforms/CMakeLists.txt to be safe
Comment Actions
Sorry, work has been postponed on this task, due to some other more important issues.
I'm planning to have some updates to this diff by end of this week.
Comment Actions
Sorry, completely forgot about this diff. Got distracted with other tasks.
I spent some time to figure out why it is not passing on my end and still 5 tests are failing, but frankly speaking - no clue. My best guess is that I might have missed some option in building, or some configuration with my machine is just causing those failures.
I'd appreciate if someone more experiance could take a look at this diff and help out in resolving issues. Thanks!
Comment Actions
can you provide the configuration you're using and the list of test failures? I'm not seeing any test failures aside from two pass manager pipeline failures with -DLLVM_EXAMPLEIRTRANSFORMS_LINK_INTO_TOOLS=ON which don't really matter
Comment Actions
While we are there, can we PLEASE rename that example pass?
Every time i look for the actual SimplifyCFG pass, i always first hit the example one...
Comment Actions
I guess the fails and unclean merge might be because I had too old sources, or just mixed something up during pulling. I have rebased it and I believe now it should be working.
Comment Actions
@aeubanks I believe this is ready for review/merge. Can you please look into this?
Personally I don't see any obvious things that might require immediate work from my end.
Comment Actions
actually I do see the failures now. I'll take care of fixing them up before landing
thanks for your work!
Comment Actions
llvm/test/Other/new-pm-O0-defaults.ll was already failing, so not gonna worry about that
I couldn't find call to createSimplifyCFGPass anywhere. In other places (e.g. llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp) similar function is actually used
// Public interface to the CFGSimplification pass FunctionPass * llvm::createCFGSimplificationPass(SimplifyCFGOptions Options, std::function<bool(const Function &)> Ftor) { return new CFGSimplifyPass(Options, std::move(Ftor)); }Do we really need this function here?