Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use llvm new pass manager api #352

Merged
merged 2 commits into from
Dec 1, 2023
Merged

Conversation

Chronostasys
Copy link
Member

No description provided.

Copy link

codecov bot commented Nov 23, 2023

Codecov Report

Merging #352 (3ad692c) into master (feb55e0) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #352      +/-   ##
==========================================
- Coverage   85.82%   85.79%   -0.03%     
==========================================
  Files         108      108              
  Lines       23987    23983       -4     
==========================================
- Hits        20587    20577      -10     
- Misses       3400     3406       +6     
Files Coverage Δ
src/ast/builder/llvmbuilder.rs 92.28% <100.00%> (-0.06%) ⬇️
src/ast/compiler.rs 95.71% <100.00%> (+0.02%) ⬆️
src/ast/node/program.rs 89.57% <100.00%> (+0.04%) ⬆️
src/ast/node/program/salsa_structs.rs 40.00% <ø> (-10.00%) ⬇️
src/lsp/mem_docs.rs 94.52% <100.00%> (-0.44%) ⬇️

... and 2 files with indirect coverage changes

Copy link

Benchmark for 45b6b7e

Click to view benchmark
Test Base PR %
allocation bench/malloc benchmark small objects 40.2±65.02ns 35.3±10.59ns -12.19%
allocation bench/singlethread gc alloc benchmark small objects 11.9±1.83ns 11.2±0.33ns -5.88%
multithreads(4) gc benchmark--65535 small objects(per thread) 1676.4±405.30µs 1599.9±305.90µs -4.56%
plimmixgc/multi-thread gc stress benchmark small objects 1199.2±35.69ms 1196.0±45.95ms -0.27%
plimmixgc/singlethread gc stress benchmark small objects 776.3±20.97ms 755.5±30.04ms -2.68%
singlethread gc benchmark--65535 small objects 769.5±11.00µs 765.2±23.88µs -0.56%

@Chronostasys Chronostasys requested review from xieyuschen and a team November 23, 2023 14:08
xieyuschen
xieyuschen previously approved these changes Nov 30, 2023
Copy link
Member

@xieyuschen xieyuschen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, could add some comments for the cpp functions

let mpm: inkwell::passes::PassManager<Module> =
unsafe { inkwell::passes::PassManager::new(ptr) };

// let ptr = unsafe { llvm_sys::core::LLVMCreatePassManager() };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove unused comments.

#include "llvm-c/Transforms/PassBuilder.h"

// param: opt opt level
extern "C" void run_module_pass(LLVMModuleRef M, int opt) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could add a comment about run_module_pass to explain:

  • what does this function do
  • which llvm API it calls(add a llvm link if possible)
  • the mapping between optimization level and integer.

@@ -30,9 +30,68 @@ extern "C" void LLVMLinkPLImmixGC()
#include "llvm-c/Transforms/PassManagerBuilder.h"

extern "C" void add_module_pass(llvm::legacy::PassManagerBase * PB) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
extern "C" void add_module_pass(llvm::legacy::PassManagerBase * PB) {
extern "C" void add_module_pass(llvm::legacy::PassManagerBase *PB) {

Comment on lines 36 to 40
#include "llvm/Passes/PassBuilder.h"
#include "llvm/Passes/PassPlugin.h"

#include "llvm-c/Types.h"
#include "llvm-c/Transforms/PassBuilder.h"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we put all files at the top of this file?

// [](const llvm::PassManagerBuilder &Builder,
// llvm::legacy::PassManagerBase &PM)
// { PM.add(new Immix()); });
// char ImmixPass::ID = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused comment

{
for (auto FB = M.functions().begin(), FE = M.functions().end(); FB != FE; ++FB)
{
Function *FV = &*FB;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious, why not use FB directly

Copy link

github-actions bot commented Dec 1, 2023

Benchmark for 98a8e28

Click to view benchmark
Test Base PR %
allocation bench/malloc benchmark small objects 35.4±20.53ns 34.2±11.08ns -3.39%
allocation bench/singlethread gc alloc benchmark small objects 11.4±1.08ns 13.3±3.26ns +16.67%
multithreads(4) gc benchmark--65535 small objects(per thread) 1641.4±78.85µs 1849.6±411.12µs +12.68%
plimmixgc/multi-thread gc stress benchmark small objects 1192.3±40.76ms 1228.2±62.58ms +3.01%
plimmixgc/singlethread gc stress benchmark small objects 759.9±26.99ms 774.3±18.12ms +1.89%
singlethread gc benchmark--65535 small objects 798.0±12.65µs 764.5±8.09µs -4.20%

@Chronostasys Chronostasys merged commit d59fad5 into master Dec 1, 2023
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants