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

Jump to bottom

Expose wasmer::{CraneliftOptLevel, LLVMOptLevel} #1894

Merged
merged 3 commits into from
Dec 17, 2020
Merged

Expose wasmer::{CraneliftOptLevel, LLVMOptLevel} #1894

merged 3 commits into from
Dec 17, 2020

Conversation

webmaster128
Copy link
Contributor

Description

This is required to do something like

use wasmer::{Cranelift, CraneliftOptLevel};

let mut compiler = Cranelift::default();
compiler.opt_level(CraneliftOptLevel::None);
let engine = JIT::new(compiler).engine();
// ...

Review

  • Add a short description of the the change to the CHANGELOG.md file

Hywan
Hywan reviewed Dec 7, 2020
View reviewed changes
lib/compiler-cranelift/src/config.rs
@@ -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.

@Hywan Hywan self-assigned this Dec 7, 2020
@Hywan Hywan added 🎉 enhancement New feature! 📦 lib-api About wasmer 📦 lib-compiler-cranelift About wasmer-compiler-cranelift labels Dec 7, 2020
@Hywan
Copy link
Contributor

Hywan commented Dec 7, 2020

Thanks for the PR!

@webmaster128 webmaster128 changed the title Expose wasmer::CraneliftOptLevel Expose wasmer::{CraneliftOptLevel, LLVMOptLevel} Dec 13, 2020
Hywan
Hywan approved these changes Dec 14, 2020
View reviewed changes
Copy link
Contributor

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Thanks!

@Hywan
Copy link
Contributor

Hywan commented Dec 14, 2020

bors r+

bors bot added a commit that referenced this pull request Dec 14, 2020
@bors @webmaster128 @Hywan
Merge #1894
c82f4ab 
1894: Expose wasmer::{CraneliftOptLevel, LLVMOptLevel} r=Hywan a=webmaster128

# Description

This is required to do something like

```rust
use wasmer::{Cranelift, CraneliftOptLevel};

let mut compiler = Cranelift::default();
compiler.opt_level(CraneliftOptLevel::None);
let engine = JIT::new(compiler).engine();
// ...
```

# Review

- [ ] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Simon Warta <simon@warta.it>
Co-authored-by: Ivan Enderlin <ivan@mnt.io>
@bors
Copy link
Contributor

bors bot commented Dec 14, 2020

Build failed:

@webmaster128
Copy link
Contributor Author

Fixed the compile issue

@Hywan
Copy link
Contributor

Hywan commented Dec 15, 2020

bors try

bors bot added a commit that referenced this pull request Dec 15, 2020
@bors
Try #1894:
cd251cc
@bors
Copy link
Contributor

bors bot commented Dec 15, 2020

try

Build succeeded:

@webmaster128
Copy link
Contributor Author

I think this is ready to be merged. Or should I update it again to master?

@Hywan
Copy link
Contributor

Hywan commented Dec 17, 2020

Please can you just update the CHANGELOG.md, since we have released the 1.0.0-beta2 it's outdated. And I'll merge it! Thanks.

@webmaster128
Expose wasmer::CraneliftOptLevel
85bb103 
This is required to do something like

```rust
use wasmer::{Cranelift, CraneliftOptLevel};

let mut compiler = Cranelift::default();
compiler.opt_level(CraneliftOptLevel::None);
let engine = JIT::new(compiler).engine();
// ...
```
@webmaster128
Expose wasmer::LLVMOptLevel
9d6fc4a
@webmaster128
Add CHANGELOG entry
50f0339
@webmaster128
Copy link
Contributor Author

Please can you just update the CHANGELOG.md, since we have released the 1.0.0-beta2 it's outdated.

Sure, done

@Hywan
Copy link
Contributor

Hywan commented Dec 17, 2020

Thanks!

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 17, 2020

Build succeeded:

@bors bors bot merged commit 8125cd3 into wasmerio:master Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hywan Hywan Hywan approved these changes

@MarkMcCaskey MarkMcCaskey Awaiting requested review from MarkMcCaskey

@nlewycky nlewycky Awaiting requested review from nlewycky

@syrusakbary syrusakbary Awaiting requested review from syrusakbary

@losfair losfair Awaiting requested review from losfair

Assignees

@Hywan Hywan

Labels
🎉 enhancement New feature! 📦 lib-api About wasmer 📦 lib-compiler-cranelift About wasmer-compiler-cranelift
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@webmaster128 @Hywan @syrusakbary