This is an archive of the discontinued LLVM Phabricator instance.

[LegacyPM] Port example pass SimplifyCFG to new PM
ClosedPublic

Authored by speryt on Oct 31 2022, 12:35 PM.

Details

Summary

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.

Diff Detail

Event Timeline

speryt created this revision.Oct 31 2022, 12:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 12:35 PM
speryt requested review of this revision.Oct 31 2022, 12:35 PM

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:

  1. 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.
  1. 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
0

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?

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:

  1. 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.

Yes we should drop everything specific to the legacy pass manager here.

  1. 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
401

I don't think the legacy pass does this, drop this part?

401

a little misleading, I'd say MadeChange instead

402

getSimplifyCFGPluginInfo?

407

formatting

421

this pass definitely doesn't preserve the CFG, we can just return PreservedAnalyses::none()

llvm/examples/IRTransforms/SimplifyCFG.h
0

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

speryt updated this revision to Diff 473046.Nov 3 2022, 3:48 PM

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.

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?

-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
40

should be able to remove this and Pass.h

379

still wrong indentation

388–389
warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
    default:

should just delete this

401

registerVectorizerStartEPCallback part should still be removed

Thanks @aeubanks for review. I should provide update this week.

speryt updated this revision to Diff 476643.Nov 18 2022, 6:08 PM

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.

fhahn added a subscriber: fhahn.Nov 20 2022, 10:19 AM

Thanks for working on this!

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

fhahn added a comment.Dec 8 2022, 9:51 AM

reverse ping :)

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.

speryt updated this revision to Diff 484225.Dec 20 2022, 5:02 AM

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!

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

also this current patch doesn't apply cleanly on main

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...

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...

I'm fine with renaming, but let's do that in a followup change

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

I'm working on Cascade Lake machine running Red Hat 8.2

speryt updated this revision to Diff 485096.Dec 23 2022, 5:28 AM

also this current patch doesn't apply cleanly on main

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.

@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.

aeubanks accepted this revision.Jan 10 2023, 3:03 PM

actually I do see the failures now. I'll take care of fixing them up before landing

thanks for your work!

This revision is now accepted and ready to land.Jan 10 2023, 3:03 PM

llvm/test/Other/new-pm-O0-defaults.ll was already failing, so not gonna worry about that

This revision was automatically updated to reflect the committed changes.
dewen added a subscriber: dewen.Mar 29 2023, 1:59 AM
llvm/examples/IRTransforms/CMakeLists.txt