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.

Sign up for GitHub

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

Expose wasmer::{CraneliftOptLevel, LLVMOptLevel} #1894

Merged
merged 3 commits into from
Dec 17, 2020
Merged

Expose wasmer::{CraneliftOptLevel, LLVMOptLevel} #1894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
85bb103
Expose wasmer::CraneliftOptLevel
webmaster128 Dec 7, 2020
9d6fc4a
Expose wasmer::LLVMOptLevel
webmaster128 Dec 13, 2020
50f0339
Add CHANGELOG entry
webmaster128 Dec 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@

## **[Unreleased]**

### Added

* [#1894](https://github.com/wasmerio/wasmer/pull/1894) Added exports `wasmer::{CraneliftOptLevel, LLVMOptLevel}` to allow using `Cranelift::opt_level` and `LLVM::opt_level` directly via the `wasmer` crate

### Changed

### Fixed

## 1.0.0-beta2 - 2020-12-16

### Added
Expand Down
4 changes: 2 additions & 2 deletions lib/api/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,10 +355,10 @@ let store = Store::new(&engine);
pub use wasmer_compiler_singlepass::Singlepass;

#[cfg(feature = "cranelift")]
pub use wasmer_compiler_cranelift::Cranelift;
pub use wasmer_compiler_cranelift::{Cranelift, CraneliftOptLevel};

#[cfg(feature = "llvm")]
pub use wasmer_compiler_llvm::LLVM;
pub use wasmer_compiler_llvm::{LLVMOptLevel, LLVM};

#[cfg(feature = "jit")]
pub use wasmer_engine_jit::{JITArtifact, JITEngine, JIT};
Expand Down
14 changes: 7 additions & 7 deletions lib/compiler-cranelift/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use wasmer_compiler::{
/// Possible optimization levels for the Cranelift codegen backend.
#[non_exhaustive]
#[derive(Clone, Debug)]
pub enum OptLevel {
pub enum CraneliftOptLevel {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to rename this type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly necessary, but with this rename we avoid naming collisions in the wasmer:: high level API namespace. In the not too far future there is probably wasmer::OptimizationLevel for the LLVM version, making it hard to reason about to which backends the types belong.

The other option would be use have pub use wasmer_compiler_cranelift::{Cranelift, OptLevel as CraneliftOptLevel};, which gives us wasmer_compiler_cranelift::OptLevel and wasmer::CraneliftOptLevel. But in my opinion this creates more confusion than it helps.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this change.
You think is likely companies will use two compilers within the same codebase? This PR moves into prefixing all the public-facing objects of compilers with CompilernameXXX

Copy link
Contributor Author

@webmaster128 webmaster128 Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You think is likely companies will use two compilers within the same codebase?

Well, we do. I don't know much about other use cases. But even if they don't: the caller code use wasmer::OptLevel; should not refer to compiler X in one project and compiler Y in a different project, right?

This PR moves into prefixing all the public-facing objects of compilers with CompilernameXXX

Right. It is based on the idea that you promoted recently: make all high level API available in the wasmer:: namespace. This is a noble goal, but hard to get right. In #1730 we were fighting imports, #1872 documents a conflict now we have at least very similar names for multiple compilers.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a good idea to rename OptLevel to CraneliftOptLevel here, since we already have the Cranelift compiler. It makes sense and it avoids name clashing. Thoughts @syrusakbary @MarkMcCaskey @nlewycky and @jubianchi?

@webmaster128 Could you extend this renaming to other compilers in this PR, so that it's consistent across the entire codebase please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@webmaster128 Could you extend this renaming to other compilers in this PR, so that it's consistent across the entire codebase please?

So far this is the only new compiler-specific export in wasmer:::

#[cfg(feature = "singlepass")]
pub use wasmer_compiler_singlepass::Singlepass;

#[cfg(feature = "cranelift")]
pub use wasmer_compiler_cranelift::{Cranelift, CraneliftOptLevel};

#[cfg(feature = "llvm")]
pub use wasmer_compiler_llvm::LLVM;

However, I now also added LLVMOptLevel to show the direction in which this is heading.

/// No optimizations performed, minimizes compilation time by disabling most
/// optimizations.
None,
Expand All @@ -33,7 +33,7 @@ pub struct Cranelift {
enable_verifier: bool,
enable_simd: bool,
enable_pic: bool,
opt_level: OptLevel,
opt_level: CraneliftOptLevel,
/// The middleware chain.
pub(crate) middlewares: Vec<Arc<dyn ModuleMiddleware>>,
}
Expand All @@ -45,7 +45,7 @@ impl Cranelift {
Self {
enable_nan_canonicalization: false,
enable_verifier: false,
opt_level: OptLevel::Speed,
opt_level: CraneliftOptLevel::Speed,
enable_pic: false,
enable_simd: true,
middlewares: vec![],
Expand All @@ -68,7 +68,7 @@ impl Cranelift {
}

/// The optimization levels when optimizing the IR.
pub fn opt_level(&mut self, opt_level: OptLevel) -> &mut Self {
pub fn opt_level(&mut self, opt_level: CraneliftOptLevel) -> &mut Self {
self.opt_level = opt_level;
self
}
Expand Down Expand Up @@ -156,9 +156,9 @@ impl Cranelift {
"none"
} else {
match self.opt_level {
OptLevel::None => "none",
OptLevel::Speed => "speed",
OptLevel::SpeedAndSize => "speed_and_size",
CraneliftOptLevel::None => "none",
CraneliftOptLevel::Speed => "speed",
CraneliftOptLevel::SpeedAndSize => "speed_and_size",
}
};

Expand Down
2 changes: 1 addition & 1 deletion lib/compiler-cranelift/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ mod trampoline;
mod translator;

pub use crate::compiler::CraneliftCompiler;
pub use crate::config::Cranelift;
pub use crate::config::{Cranelift, CraneliftOptLevel};
pub use crate::debug::{ModuleInfoMemoryOffset, ModuleInfoVmctxInfo, ValueLabelsRanges};
pub use crate::trampoline::make_trampoline_function_call;

Expand Down
8 changes: 4 additions & 4 deletions lib/compiler-llvm/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use inkwell::targets::{
CodeModel, InitializationConfig, RelocMode, Target as InkwellTarget, TargetMachine,
TargetTriple,
};
use inkwell::OptimizationLevel;
pub use inkwell::OptimizationLevel as LLVMOptLevel;
use itertools::Itertools;
use std::fmt::Debug;
use std::sync::Arc;
Expand Down Expand Up @@ -41,7 +41,7 @@ pub trait LLVMCallbacks: Debug + Send + Sync {
pub struct LLVM {
pub(crate) enable_nan_canonicalization: bool,
pub(crate) enable_verifier: bool,
pub(crate) opt_level: OptimizationLevel,
pub(crate) opt_level: LLVMOptLevel,
is_pic: bool,
pub(crate) callbacks: Option<Arc<dyn LLVMCallbacks>>,
/// The middleware chain.
Expand All @@ -55,7 +55,7 @@ impl LLVM {
Self {
enable_nan_canonicalization: false,
enable_verifier: false,
opt_level: OptimizationLevel::Aggressive,
opt_level: LLVMOptLevel::Aggressive,
is_pic: false,
callbacks: None,
middlewares: vec![],
Expand All @@ -72,7 +72,7 @@ impl LLVM {
}

/// The optimization levels when optimizing the IR.
pub fn opt_level(&mut self, opt_level: OptimizationLevel) -> &mut Self {
pub fn opt_level(&mut self, opt_level: LLVMOptLevel) -> &mut Self {
self.opt_level = opt_level;
self
}
Expand Down
4 changes: 3 additions & 1 deletion lib/compiler-llvm/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,6 @@ mod trampoline;
mod translator;

pub use crate::compiler::LLVMCompiler;
pub use crate::config::{CompiledKind, InkwellMemoryBuffer, InkwellModule, LLVMCallbacks, LLVM};
pub use crate::config::{
CompiledKind, InkwellMemoryBuffer, InkwellModule, LLVMCallbacks, LLVMOptLevel, LLVM,
};