Skip to content

cranelift: Implement float rounding operations in interpreter#4397

Merged
cfallin merged 1 commit intobytecodealliance:mainfrom
afonso360:interp-rounding
Jul 6, 2022
Merged

cranelift: Implement float rounding operations in interpreter#4397
cfallin merged 1 commit intobytecodealliance:mainfrom
afonso360:interp-rounding

Conversation

@afonso360
Copy link
Contributor

👋 Hey,

This PR implements the following operations on the interpreter:

  • ceil
  • floor
  • nearest
  • trunc

@afonso360 afonso360 changed the title cranelift: Implement float rounding operations cranelift: Implement float rounding operations in interpreter Jul 6, 2022
@github-actions github-actions bot added the cranelift Issues related to the Cranelift code generator label Jul 6, 2022
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Thanks!

@cfallin
Copy link
Member

cfallin commented Jul 6, 2022

Merge conflict, happy to merge once resolved!

Implements the following operations on the interpreter:
* `ceil`
* `floor`
* `nearest`
* `trunc`
@cfallin cfallin merged commit f98076a into bytecodealliance:main Jul 6, 2022

/// Returns the nearest integer to `self`. Round half-way cases away from `0.0`.
pub fn nearest(self) -> Self {
Self::with_float(self.as_f32().round())
Copy link
Contributor

@MaxGraey MaxGraey Jul 7, 2022

Choose a reason for hiding this comment

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

round is not the same as nearest. I think I once implemented the correct & efficient "nearest" in the past: #2171

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry I missed this... @afonso360 could you look into this further?

Copy link
Contributor

Choose a reason for hiding this comment

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

Rust has not had this operation for very many years. Here it seems someone finally want to add it:
rust-lang/rust#95317

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow! Great catch!

Is there a simple way to organize the code so that we can reuse that implementation?

I'm not sure we can use nightly features in wasmtime/cranelift.

Otherwise we can probably duplicate the function pending stabilization of the rust implementation.

afonso360 added a commit to afonso360/wasmtime that referenced this pull request Jul 7, 2022
 As @MaxGraey pointed out (thanks!) in bytecodealliance#4397, `round` has different
 behavior from `nearest`. And it looks like the native rust
 implementation is still pending stabilization.

 Right now we duplicate the wasmtime implementation, merged in bytecodealliance#2171.

 However, we definitely should switch to the rust native version
 when it is available.
cfallin pushed a commit that referenced this pull request Jul 7, 2022
As @MaxGraey pointed out (thanks!) in #4397, `round` has different
 behavior from `nearest`. And it looks like the native rust
 implementation is still pending stabilization.

 Right now we duplicate the wasmtime implementation, merged in #2171.

 However, we definitely should switch to the rust native version
 when it is available.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants