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: RISC-V wrong result for fcvt_to_sint_sat.{i8,i16} #5993

Closed
afonso360 opened this issue Mar 11, 2023 · 1 comment
Closed

Cranelift: RISC-V wrong result for fcvt_to_sint_sat.{i8,i16} #5993

afonso360 opened this issue Mar 11, 2023 · 1 comment
Labels
bug Incorrect behavior in the current implementation that needs fixing cranelift:area:riscv64 Issues related to the RISC-V 64 backend. cranelift Issues related to the Cranelift code generator

Comments

@afonso360
Copy link
Contributor

afonso360 commented Mar 11, 2023 •
edited
Loading

👋 Hey,

.clif Test Case

test interpret
test run
target riscv64gc

function %a(f64) -> i8 {
block0(v0: f64):
    v1 = fcvt_to_sint_sat.i8 v0
    return v1
}
; run: %a(-0x1.811d818400000p30) == -128

function %b(f64) -> i16 {
block0(v0: f64):
    v1 = fcvt_to_sint_sat.i16 v0
    return v1
}
; run: %b(-0x1.811d818400000p30) == -32768

Steps to Reproduce

  • clif-util test ./the-above.clif

Expected Results

The test to pass

Actual Results

For %a we get:

 ERROR cranelift_filetests::concurrent > FAIL: run
FAIL ./lmao.clif: run

Caused by:
    Failed test: run: %a(-0x1.811d818400000p30) == -128, actual: -97
1 tests
Error: 1 failure

For %b we get:

 ERROR cranelift_filetests::concurrent > FAIL: run
FAIL ./lmao.clif: run

Caused by:
    Failed test: run: %a(-0x1.811d818400000p30) == -32768, actual: -24673
1 tests
Error: 1 failure

Versions and Environment

Cranelift version or commit: main

Operating system: Linux

Architecture: riscv64

Extra Info

cc: @yuyang-ok
@afonso360 afonso360 added bug Incorrect behavior in the current implementation that needs fixing cranelift Issues related to the Cranelift code generator cranelift:area:riscv64 Issues related to the RISC-V 64 backend. labels Mar 11, 2023
@afonso360 afonso360 changed the title Cranelift: RISC-V wrong result for fcvt_to_sint_sat.i8 Cranelift: RISC-V wrong result for fcvt_to_sint_sat.{i8,i16} Mar 11, 2023
@yuyang-ok yuyang-ok mentioned this issue Mar 14, 2023
Closed
@afonso360 afonso360 mentioned this issue Oct 23, 2023
Merged
alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Oct 23, 2023
@alexcrichton
Add regression tests and bless output
22fa6f7 
Closes bytecodealliance#5992
Closes bytecodealliance#5993
github-merge-queue bot pushed a commit that referenced this issue Oct 23, 2023
@alexcrichton
riscv64: Refactor FRM and fcvt-to-int management (#7327)
72aaff5 
* riscv64: Specify rounding modes in instructions

This commit updates how floating-point instructions specify their float
rounding mode (FRM). Previously instructions stored `Option<FRM>` and
this would mostly be `None`. All floating-point instructions in RISC-V
have a 3-bit `rm` field, and most encode the FRM into this field but
some have a require encoding of this field. For example `fsgnj.s` uses
the `rm` field to differentiate between `fsgnj`, `fsgnjx`, and `fsgnjn`.
Instructions like `fadd` however use this field for a rounding mode.

All FPU instructions now store `FRM` directly. Instruction helpers like
`fadd` require this to be specified explicitly. Instructions helpers
like for `fsgnj` do not take this as an argument and hardcode the field
as necessary. This means that all lowerings of floating point
instructions, where relevant, now specify a rounding mode.

Previously the default rounding mode was to use the `fcsr` register,
meaning that the rounding mode would be determined dynamically at
runtime depending on the status of this register. Cranelift semantics,
however, are derivative of WebAssembly semantics which specify
round-to-nearest ties-to-even. This PR additionally fixes this
discrepancy by using `FRM::RNE` in all existing instructions instead of
`FRM::Fcsr`.

* riscv64: Refactor float-to-int conversions

This commit removes the `FcvtToInt` macro-instruction in the riscv64
backend in favor of decomposing it into individual operation for
`fcvt_to_{s,u}int*` instructions. This additionally provides a slightly
different lowering for the `*_sat` operations which doesn't use
branches. The non-saturating operations continue to have a number of
branches and their code has changed slightly due to how immediates are
loaded. Overall everything is in ISLE now instead of split a bit.

* riscv64: Clean up some dead code in the backend

Don't put `#![allow(dead_code)]` at the root, instead place it on some
smaller items.

* Fix emission tests

* Add regression tests and bless output

Closes #5992
Closes #5993

* Enable i8/i16 saturating float-to-int in fuzzgen

* Better `fcvt_*_bound` implementations

* Fix typo in match orderings

* Fix tests on x64

Where float-to-int isn't implemented for i8/i16
@afonso360
Copy link
Contributor Author

Fixed in #7327

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Incorrect behavior in the current implementation that needs fixing cranelift:area:riscv64 Issues related to the RISC-V 64 backend. cranelift Issues related to the Cranelift code generator
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@afonso360