GH-37834: [Gandiva] Migrate to new LLVM PassManager API #37867

niyue · 2023-09-26T02:30:57Z

Rationale for this change

In #37834, to support LLVM 17, we need to migrate to use the new LLVM PassManager API.

What changes are included in this PR?

This PR tries to migrate the legacy PassManager to the new PassManager.

Are these changes tested?

It should be covered by existing unit tests. But more performance tests may be needed to verify this change.

Are there any user-facing changes?

No

Closes: [C++][Gandiva] Support LLVM 17 #37834

github-actions · 2023-09-26T02:31:30Z

⚠️ GitHub issue #37834 has been automatically assigned in GitHub to PR creator.

kou · 2023-09-26T07:31:15Z

Could you also apply #37834 (comment) to accept LLVM 17?

kou · 2023-09-26T07:32:49Z

We have some micro benchmarkes in https://github.com/apache/arrow/blob/main/cpp/src/gandiva/tests/micro_benchmarks.cc . Can we use them?

niyue · 2023-09-26T09:58:09Z

We have some micro benchmarkes in https://github.com/apache/arrow/blob/main/cpp/src/gandiva/tests/micro_benchmarks.cc . Can we use them?

Let me check it out. I've compared the old/new LLVM IR generated by the particular test code today, and they are exactly the same (at least for this test case), so I assume the change is probably not too risky.

niyue · 2023-09-26T10:08:41Z

Could you also apply #37834 (comment) to accept LLVM 17?

I applied your patch for LLVM 17 a moment ago, and it should help to address some issues in the CI.

LLVM incompatible API changes (or new API)

But I realized my current changes use some APIs that are not available in lower version of LLVM, for example, this CI check failed (https://github.com/apache/arrow/actions/runs/6307346371/job/17123835292?pr=37867), there are at least 4 incompatible changes:

llvm::GVNPass() (I confirmed this is a newly added API in Nov. 10, 2021 [2], introduced in LLVM 14.0.0)
llvm::ModuleInlinerPass() (introduced in Nov. 9, 2021 [3], in LLVM 14.0.0)
llvm::OptimizationLevel (introduced in July, 30, 2021 [4], in LLVM 13.0.0 likely)
cannot convert 'gandiva::Engine::FinalizeModule()::<lambda(llvm::ModulePassManager&, int)>' to 'const std::function<void(llvm::PassManager<llvm::Module>&)>&'

I used LLVM 14.0.6 locally.
For such cases, do we need to use macro to call different APIs? Currently, the lowest version of LLVM listed in the support list is 7.0, we probably won't be able to verify many different versions of LLVMs, one option is we use two implementations for PassManager:

the legacy PassManager one for LLVM version < 14 (the old implementation)
the new PassManager for LLVM version >= 14 (the implementation in this PR)

@kou what do you think?

linker error

This CI check (https://github.com/apache/arrow/actions/runs/6307346377/job/17123834680?pr=37867) failed with some linker error undefined reference, but it is not a compiler error, do we have multiple version of LLVMs installed in the environment?

links

[1] LLVM release dates, https://releases.llvm.org
[2] https://github.com/llvm/llvm-project/blame/7675f541f75baa20e8ec007cd625a837e89fc01f/llvm/include/llvm/Transforms/Scalar/GVN.h#L117
[3] https://github.com/llvm/llvm-project/blame/5c6eefb252e3b16191fac684de7285812769778d/llvm/include/llvm/Transforms/IPO/ModuleInliner.h#L27
[4] https://github.com/llvm/llvm-project/blame/5c6eefb252e3b16191fac684de7285812769778d/llvm/include/llvm/Passes/OptimizationLevel.h#L22

kou · 2023-09-26T20:38:14Z

I've compared the old/new LLVM IR generated by the particular test code today, and they are exactly the same

Great!

For such cases, do we need to use macro to call different APIs? Currently, the lowest version of LLVM listed in the support list is 7.0, we probably won't be able to verify many different versions of LLVMs, one option is we use two implementations for PassManager:
1. the legacy PassManager one for LLVM version < 14 (the old implementation)
2. the new PassManager for LLVM version >= 14 (the implementation in this PR)

Is it something like the following?

diff --git a/cpp/src/gandiva/engine.cc b/cpp/src/gandiva/engine.cc
index 6abe81405..b5104c5b7 100644
--- a/cpp/src/gandiva/engine.cc
+++ b/cpp/src/gandiva/engine.cc
@@ -308,6 +308,7 @@ Status Engine::FinalizeModule() {
     ARROW_RETURN_NOT_OK(RemoveUnusedFunctions());
 
     if (optimize_) {
+#if LLVM_VERSION_MAJOR >= 14
       // Setup an optimiser pipeline
       llvm::PassBuilder pass_builder;
       llvm::LoopAnalysisManager loop_am;
@@ -345,6 +346,30 @@ Status Engine::FinalizeModule() {
 
       pass_builder.buildPerModuleDefaultPipeline(llvm::OptimizationLevel::O3)
           .run(*module_, module_am);
+#else
+      // misc passes to allow for inlining, vectorization, ..
+      std::unique_ptr<llvm::legacy::PassManager> pass_manager(
+          new llvm::legacy::PassManager());
+
+      llvm::TargetIRAnalysis target_analysis =
+          execution_engine_->getTargetMachine()->getTargetIRAnalysis();
+      pass_manager->add(llvm::createTargetTransformInfoWrapperPass(target_analysis));
+      pass_manager->add(llvm::createFunctionInliningPass());
+      pass_manager->add(llvm::createInstructionCombiningPass());
+      pass_manager->add(llvm::createPromoteMemoryToRegisterPass());
+      pass_manager->add(llvm::createGVNPass());
+      pass_manager->add(llvm::createNewGVNPass());
+      pass_manager->add(llvm::createCFGSimplificationPass());
+      pass_manager->add(llvm::createLoopVectorizePass());
+      pass_manager->add(llvm::createSLPVectorizerPass());
+      pass_manager->add(llvm::createGlobalOptimizerPass());
+
+      // run the optimiser
+      llvm::PassManagerBuilder pass_builder;
+      pass_builder.OptLevel = 3;
+      pass_builder.populateModulePassManager(*pass_manager);
+      pass_manager->run(*module_);
+#endif
     }
 
     ARROW_RETURN_IF(llvm::verifyModule(*module_, &llvm::errs()),

I like the approach.

linker error

This CI check (https://github.com/apache/arrow/actions/runs/6307346377/job/17123834680?pr=37867) failed with some linker error undefined reference, but it is not a compiler error, do we have multiple version of LLVMs installed in the environment?

It uses static linking. I think that we need to add more components for the new PassManager API at:

arrow/cpp/cmake_modules/FindLLVMAlt.cmake

Lines 89 to 98 in 23c1bf9

    
           llvm_map_components_to_libnames(LLVM_LIBS 
        
                                           core 
        
                                           mcjit 
        
                                           native 
        
                                           ipo 
        
                                           bitreader 
        
                                           target 
        
                                           linker 
        
                                           analysis 
        
                                           debuginfodwarf)

If you're not familiar with CMake, I can do it. But I can't work on it in this week...

niyue · 2023-09-27T00:47:49Z

Is it something like the following?

Yes, I am thinking something like you said.

It uses static linking. I think that we need to add more components for the new PassManager API at

Cool. I am not aware that the new PassManager needs to change LLVM components. Why this issue doesn't happen in other CI environments? Anyway, let me give it a try to see if I can figure it out by adding some more components.

niyue · 2023-09-27T01:45:04Z

FinalizeModule

For the FinalizeModule method, I added back the old implementation using the legacy pass manager for LLVM version < 14 for compatibility.

RemoveUnusedFunctions

For the RemoveUnusedFunctions method, I verified the APIs used and they all seem introduced long time ago, and there should not be compatibility issue, but I don't actually verify against LLVM 7.0, but only check github history for verification.

Here are the major APIs used in RemoveUnusedFunctions method:

CMake LLVM components change

I didn't change the CMake LLVM components yet, since if we use legacy LLVM PassManager API for old versions of LLVM, we probably don't need to change it any more. I will keep an eye on the CI results to see if it works for LLVM 17 environments.

kou · 2023-09-27T02:22:24Z

Sorry. Only for the linker error: We may be able to reproduce the linker error on local with -DARROW_LLVM_USE_SHARED=OFF.

…nager API so that the code can work with multiple versions of LLVM.

niyue · 2023-09-28T02:29:14Z

After switching to use 2 implementations for legacy/new PassManager API, there are still 2 relevant CI checks failed:

Both seemed to be related with LLVM linking issue. I will do some further investigation but may have to be off for a few days

kou

+1

I've fixed the remained CI failures.

cpp/src/gandiva/engine.cc

conbench-apache-arrow · 2023-10-03T10:26:32Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit d428ef4.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

niyue · 2023-10-05T05:08:33Z

@kou thanks so much for fixing the remaining issues!

…#37867) ### Rationale for this change In apache#37834, to support LLVM 17, we need to migrate to use the new LLVM PassManager API. ### What changes are included in this PR? This PR tries to migrate the legacy PassManager to the new PassManager. ### Are these changes tested? It should be covered by existing unit tests. But more performance tests may be needed to verify this change. ### Are there any user-facing changes? No * Closes: apache#37834 Lead-authored-by: Yue Ni <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

github-actions bot added Component: C++ Component: Gandiva awaiting review Awaiting review labels Sep 26, 2023

niyue mentioned this pull request Sep 26, 2023

[C++][Gandiva] Support LLVM 17 #37834

Closed

niyue force-pushed the feature/new-passmanager branch from 4c34593 to b0fe18a Compare September 26, 2023 09:56

niyue force-pushed the feature/new-passmanager branch from b0fe18a to 6c514dc Compare September 27, 2023 01:38

niyue force-pushed the feature/new-passmanager branch 2 times, most recently from 6c93b96 to f3048cf Compare September 28, 2023 00:56

niyue added 4 commits September 28, 2023 08:57

Migrate RemoveUnusedFunctions to use the new PassManager API.

70e87f9

Update the FinalizeModule function to use the new LLVM PassManager API.

3947885

Support LLVM 17 for Gandiva.

ba80d88

Adapt the module optimization to use different version of LLVM PassMa…

99a0909

…nager API so that the code can work with multiple versions of LLVM.

niyue force-pushed the feature/new-passmanager branch from f3048cf to 99a0909 Compare September 28, 2023 00:57

Use passes for new PassManager API

7d775c8

kou approved these changes Oct 1, 2023

View reviewed changes

cpp/src/gandiva/engine.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Oct 1, 2023

Use more meaningful name

5e5fe68

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Oct 3, 2023

kou mentioned this pull request Oct 3, 2023

GH-37635: [Format][C++][Go] Add app_metadata to FlightInfo and FlightEndpoint #37679

Merged

kou merged commit d428ef4 into apache:main Oct 3, 2023
30 checks passed

kou removed the awaiting changes Awaiting changes label Oct 3, 2023

niyue deleted the feature/new-passmanager branch October 5, 2023 05:09

bkietz mentioned this pull request Oct 23, 2023

GH-37796: [C++][Acero] Fix race condition caused by straggling input in the as-of-join node #37839

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-37834: [Gandiva] Migrate to new LLVM PassManager API #37867

GH-37834: [Gandiva] Migrate to new LLVM PassManager API #37867

niyue commented Sep 26, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Sep 26, 2023

kou commented Sep 26, 2023

kou commented Sep 26, 2023

niyue commented Sep 26, 2023

niyue commented Sep 26, 2023 •

edited

Loading

kou commented Sep 26, 2023

linker error

niyue commented Sep 27, 2023

niyue commented Sep 27, 2023 •

edited

Loading

kou commented Sep 27, 2023 •

edited

Loading

niyue commented Sep 28, 2023

kou left a comment

conbench-apache-arrow bot commented Oct 3, 2023

niyue commented Oct 5, 2023

GH-37834: [Gandiva] Migrate to new LLVM PassManager API #37867

GH-37834: [Gandiva] Migrate to new LLVM PassManager API #37867

Conversation

niyue commented Sep 26, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Sep 26, 2023

kou commented Sep 26, 2023

kou commented Sep 26, 2023

niyue commented Sep 26, 2023

niyue commented Sep 26, 2023 • edited Loading

LLVM incompatible API changes (or new API)

linker error

links

kou commented Sep 26, 2023

linker error

niyue commented Sep 27, 2023

niyue commented Sep 27, 2023 • edited Loading

FinalizeModule

RemoveUnusedFunctions

PassBuilder.registerModuleAnalyses

ModulePassManager

InternalizePass

GlobalDCEPass

CMake LLVM components change

kou commented Sep 27, 2023 • edited Loading

niyue commented Sep 28, 2023

kou left a comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Oct 3, 2023

niyue commented Oct 5, 2023

niyue commented Sep 26, 2023 •

edited by github-actions bot

Loading

niyue commented Sep 26, 2023 •

edited

Loading

niyue commented Sep 27, 2023 •

edited

Loading

kou commented Sep 27, 2023 •

edited

Loading