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

Cranelift upgrade #1781

Merged
merged 6 commits into from
Oct 30, 2020
Merged

Cranelift upgrade #1781

merged 6 commits into from
Oct 30, 2020

Conversation

syrusakbary
Copy link
Member

Description

Upgrade Cranelift to 0.67. This upgrade also enables all SIMD tests (with one small exception operator that is still not fixed in Cranelift).

nlewycky
nlewycky approved these changes Oct 30, 2020
View reviewed changes
lib/compiler-cranelift/src/func_environ.rs
) -> WasmResult<ir::Value> {
Ok(match ty {
Type::FuncRef => pos.ins().iconst(self.pointer_type(), 0),
// Type::ExternRef => pos.ins().null(self.reference_type(ty)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: generally we don't check in commented out code.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be useful when we add reference types. I think it will be useful to keep it!

lib/compiler-cranelift/src/translator/code_translator.rs Outdated Show resolved Hide resolved
lib/compiler-cranelift/src/translator/code_translator.rs
@@ -1291,15 +1381,27 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
)?;
let splatted = builder.ins().splat(type_of(op), state.pop1());
state.push1(splatted)

// let opcode = ir::Opcode::LoadSplat;
Copy link
Contributor

Choose a reason for hiding this comment

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

commented out code again

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be useful for the future (is directly ported from cranelift-wasm, but can't be used just yet since it requires for Cranelift to publish a new version), that's why is useful keeping it!

lib/compiler-cranelift/src/translator/code_translator.rs
@@ -1460,6 +1566,11 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
let bool_result = builder.ins().vall_true(a);
state.push1(builder.ins().bint(I32, bool_result))
}
Operator::I8x16Bitmask | Operator::I16x8Bitmask | Operator::I32x4Bitmask => {
unimplemented!("SIMD Operator {:?} not yet implemented", op);
// let a = pop1_with_bitcast(state, type_of(op), builder);
Copy link
Contributor

Choose a reason for hiding this comment

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

commented-out code

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be useful for the future (is directly ported from cranelift-wasm, but can't be used just yet since it requires for Cranelift to publish a new version), that's why is useful keeping it!

lib/compiler-cranelift/src/translator/code_translator.rs
@@ -1535,6 +1646,21 @@ pub fn translate_operator<FE: FuncEnvironment + ?Sized>(
let (a, b) = pop2_with_bitcast(state, type_of(op), builder);
state.push1(builder.ins().fmin(a, b))
}
Operator::F32x4PMax | Operator::F64x2PMax => {
unimplemented!("SIMD Operator {:?} not yet implemented", op);
// let (a, b) = pop2_with_bitcast(state, type_of(op), builder);
Copy link
Contributor

Choose a reason for hiding this comment

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

commented-out code here and a few lines later

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be useful from the future (is directly ported from cranelift-wasm), that's why is useful keeping it!

lib/compiler-cranelift/src/translator/code_translator.rs
@@ -1894,7 +2062,7 @@ fn mem_op_size(opcode: ir::Opcode, ty: Type) -> u32 {
ir::Opcode::Istore8 | ir::Opcode::Sload8 | ir::Opcode::Uload8 => 1,
ir::Opcode::Istore16 | ir::Opcode::Sload16 | ir::Opcode::Uload16 => 2,
ir::Opcode::Istore32 | ir::Opcode::Sload32 | ir::Opcode::Uload32 => 4,
ir::Opcode::Store | ir::Opcode::Load => ty.bytes(),
ir::Opcode::Store | ir::Opcode::Load /* | ir::Opcode::LoadSplat */=> ty.bytes(),
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 this would be wrong for LoadSplat -- LoadSplat loads a small size (1 byte / 2 byte / 4 byte / 8 byte) and splats it to the full 16 byte. If you use the type of the value pushed onto the stack by the loadsplat instruction then you would always return 16 for the V128. The function is supposed to return how many bytes it loads/stores.

Copy link
Member Author

@syrusakbary syrusakbary Oct 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's directly ported from cranelift-wasm, but can't be used just yet

lib/compiler-cranelift/src/translator/code_translator.rs Outdated
Comment on lines 2576 to 2579
match ty {
B8X16 | B16X8 | B32X4 | B64X2 | I64X2 | I32X4 | I16X8 | F32X4 | F64X2 => true,
_ => false,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use matches!.

@syrusakbary
Copy link
Member Author

bors r+

@syrusakbary
Copy link
Member Author

bors r-

@bors
Copy link
Contributor

bors bot commented Oct 30, 2020

Canceled.

@syrusakbary
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Oct 30, 2020
@bors @syrusakbary
Merge #1781
d80ad11 
1781: Cranelift upgrade r=syrusakbary a=syrusakbary

<!-- 
Prior to submitting a PR, review the CONTRIBUTING.md document for recommendations on how to test:
https://github.com/wasmerio/wasmer/blob/master/CONTRIBUTING.md#pull-requests

-->

# Description

Upgrade Cranelift to `0.67`. This upgrade also enables all SIMD tests (with one small exception operator that is still not fixed in Cranelift).

<!-- 
Provide details regarding the change including motivation,
links to related issues, and the context of the PR.
-->


Co-authored-by: Syrus <[email protected]>
Co-authored-by: Syrus Akbary <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 30, 2020

Build failed:

@syrusakbary
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 30, 2020

Build succeeded:

@bors bors bot merged commit 1a4cd32 into master Oct 30, 2020
@bors bors bot deleted the cranelift-upgrade branch October 30, 2020 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlewycky nlewycky nlewycky approved these changes

@losfair losfair Awaiting requested review from losfair

@MarkMcCaskey MarkMcCaskey Awaiting requested review from MarkMcCaskey

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@syrusakbary @nlewycky