Codegen fix convert float to int on small types with riscv64. #6015

yuyang-ok · 2023-03-14T05:20:24Z

This will fix #5992 and #5993
We should normalize float type when convert float to int on {i8,i16}.
Because riscv64 has no direct instruction to support it.

yuyang-ok · 2023-03-14T06:11:04Z

@afonso360 Looks like The test failure indicates x64 not implement for small types.

cranelift/codegen/src/isa/riscv64/lower/isle.rs

tests/spec_testsuite

cranelift/codegen/src/isa/riscv64/inst.isle

…n ISLE.

yuyang-ok · 2023-03-15T03:23:52Z

@afonso360 I have rewritten to ISLE .
fmax_ is That I can't figure out a better name.
Maybe you can review code first.

afonso360

It's looking better! I just noticed that we are applying the clamping to all fcvt_ rules, and I suspect that is going to break the other ones.

I'm going to run the fuzzer on this for a while to double check.

Also @jameysharp could you also take a look at this PR?

Edit: The fuzzer found the following:

Testcase

test interpret
test run
target riscv64gc

function %a(f64) -> i16 sext system_v {
block0(v6: f64):
    v16 = fcvt_to_sint_sat.i16 v6
    return v16
}

; run: %a(-NaN:0x7ffffff666666) == 0

Results

FAIL ./lmao.clif: run

Caused by:
    Failed test: run: %a(-NaN:0x7ffffff666666) == 0, actual: -32768
1 tests
Error: 1 failure

I'll see if I can get an example that is exclusive to the other fcvt instructions

cranelift/codegen/src/isa/riscv64/inst.isle

afonso360 · 2023-03-15T16:37:34Z

cranelift/codegen/src/isa/riscv64/inst.isle

@@ -1798,7 +1798,8 @@
  (let
    ((result WritableReg (temp_writable_reg out_type))
      (tmp WritableReg (temp_writable_reg $F64))
-      (_ Unit (emit (MInst.FcvtToInt is_sat result tmp rs is_signed in_type out_type))))
+      (rs2 Reg (clamp_fcvt_from_float in_type out_type rs is_signed))


This seems to target the whole family of fcvt_* instructions, but the clamp here is really only needed for fcvt_*_sat which do the saturating conversions.

afonso360 · 2023-03-15T16:42:54Z

cranelift/codegen/src/isa/riscv64/inst.isle

@@ -2195,6 +2223,14 @@
 (rule (flt $F32 a b) (fpu_rrr (FpuOPRRR.FltS) $I64 a b))
 (rule (flt $F64 a b) (fpu_rrr (FpuOPRRR.FltD) $I64 a b))

+(decl fmin_ (Type Reg Reg) Reg)


Looks like we can't name this fmin since it conflicts with the preexisting declaration for the fmin clif instruction. I guess adding a prefix to all RISCV instruction helpers would make sense? The x86 backend has x64_* so we could add something like rv_*, what do you guys think about this?

If everyone agrees I don't mind doing a pass on the backend adding the helpers and making sure everything is aligned.

I think a prefix like rv_ is a good idea. But I also think it would be okay to merge this with the fmin_/fmax_ names, and clean it up later.

afonso360 · 2023-03-15T16:44:55Z

cranelift/codegen/src/isa/riscv64/inst.isle

+    (max_reg Reg (imm in_ty (i32_2_float_bits max_value in_ty))))
+    (fmin_ in_ty tmp max_reg)))
+
+(decl i8_i16_max_value (Type bool) i32)


I guess I have a slight preference for having these on ISLE as well, but I'm not too sure. @jameysharp what do you think about this?

In general I agree that I'd like to do as much in ISLE as possible.

I think there are a number of tricks we can use which might clean this up, but I'm having trouble putting them together into a complete suggestion.

The pure ISLE version of this probably would return the appropriate hex-literal float for each combination of type and is_signed. I'm not sure how easy that'll be to write, understand, or maintain.

Leaving some of the work to Rust, we can factor it differently to shorten the code. With signed conversion, we can start with i64::MAX or i64::MIN, then x >> (64 - ty.bits()), then use f32_bits(x as f32) or f64_bits(x as f64). Unsigned is the same except for starting with u64::MAX or u64::MIN and using unsigned shifts. Since we apparently only need to clamp for fits_in_16 types (why is that though?) we can replace 64 with 16 in the first two steps.

Since we apparently only need to clamp for fits_in_16 types (why is that though?)

We have native instructions for this operation in both 32 and 64 bit variants, but for <16 we clamp the values to the maximum range supported by i8/i16 and then do the 32 bit conversion.

cranelift/codegen/src/isa/riscv64/inst.isle

jameysharp

I'm not sure I have super helpful advice. Mostly I can say that @afonso360, I think your feedback makes sense.

jameysharp · 2023-03-15T21:50:38Z

cranelift/codegen/src/isa/riscv64/inst.isle

@@ -2195,6 +2223,14 @@
 (rule (flt $F32 a b) (fpu_rrr (FpuOPRRR.FltS) $I64 a b))
 (rule (flt $F64 a b) (fpu_rrr (FpuOPRRR.FltD) $I64 a b))

+(decl fmin_ (Type Reg Reg) Reg)


I think a prefix like rv_ is a good idea. But I also think it would be okay to merge this with the fmin_/fmax_ names, and clean it up later.

cranelift/codegen/src/isa/riscv64/inst.isle

jameysharp · 2023-03-15T22:27:08Z

cranelift/codegen/src/isa/riscv64/inst.isle

+    (max_reg Reg (imm in_ty (i32_2_float_bits max_value in_ty))))
+    (fmin_ in_ty tmp max_reg)))
+
+(decl i8_i16_max_value (Type bool) i32)


In general I agree that I'd like to do as much in ISLE as possible.

I think there are a number of tricks we can use which might clean this up, but I'm having trouble putting them together into a complete suggestion.

The pure ISLE version of this probably would return the appropriate hex-literal float for each combination of type and is_signed. I'm not sure how easy that'll be to write, understand, or maintain.

Leaving some of the work to Rust, we can factor it differently to shorten the code. With signed conversion, we can start with i64::MAX or i64::MIN, then x >> (64 - ty.bits()), then use f32_bits(x as f32) or f64_bits(x as f64). Unsigned is the same except for starting with u64::MAX or u64::MIN and using unsigned shifts. Since we apparently only need to clamp for fits_in_16 types (why is that though?) we can replace 64 with 16 in the first two steps.

Co-authored-by: Afonso Bordado <afonso360@users.noreply.github.com>

Co-authored-by: Jamey Sharp <jamey@minilop.net>

…o issue5992

…nto issue5992

yuyang-ok · 2023-04-22T00:10:41Z

@afonso360 any idea about this failure.

test interpret
test run
target riscv64gc

function %a(f64) -> i16 sext system_v {
block0(v6: f64):
    v16 = fcvt_to_sint_sat.i16 v6
    return v16
}

; run: %a(-NaN:0x7ffffff666666) == 0

afonso360 · 2023-04-23T14:39:31Z

Well, looks like we are not returning 0 for all NaN's. When testing this locally I got run: %b(-NaN:0x7ffffff666666) == 0, actual: -32768 so it seems like something is going wrong in the NaN checking.

Disassembly of the test case

``` Disassembly of 112 bytes: 0: 97 06 00 00 auipc a3, 0 4: 83 b6 c6 00 ld a3, 0xc(a3) 8: 6f 00 c0 00 j 0xc c: 00 00 00 00 .byte 0x00, 0x00, 0x00, 0x00 10: 00 00 e0 c0 .byte 0x00, 0x00, 0xe0, 0xc0 14: 53 83 06 f2 fmv.d.x ft6, a3 18: d3 15 a3 2a fmax.d fa1, ft6, fa0 1c: 97 0e 00 00 auipc t4, 0 20: 83 be ce 00 ld t4, 0xc(t4) 24: 6f 00 c0 00 j 0xc 28: 00 00 00 00 .byte 0x00, 0x00, 0x00, 0x00 2c: c0 ff df 40 .byte 0xc0, 0xff, 0xdf, 0x40 30: 53 87 0e f2 fmv.d.x fa4, t4 34: d3 88 e5 2a fmin.d fa7, fa1, fa4 38: 53 a8 18 a3 feq.d a6, fa7, fa7 3c: 63 02 08 02 beqz a6, 0x24 40: 53 98 08 c2 fcvt.w.d a6, fa7, rtz 44: 37 8f 00 00 lui t5, 8 48: 93 0f ff ff addi t6, t5, -1 4c: b3 7f f8 01 and t6, a6, t6 50: 13 58 f8 01 srli a6, a6, 0x1f 54: 13 18 f8 00 slli a6, a6, 0xf 58: 33 68 f8 01 or a6, a6, t6 5c: 6f 00 80 00 j 8 60: 13 08 00 00 mv a6, zero 64: 13 15 08 03 slli a0, a6, 0x30 68: 13 55 05 43 srai a0, a0, 0x30 6c: 67 80 00 00 ret ```

I haven't looked too close at this, but it looks like we only do the NaN check after the value is clamped, so could that be the reason for the issues?

I also tested building an equivalent test case using LLVM It looks like they do the NaN check on the original input.

…nto issue5992

yuyang-ok · 2023-04-24T04:08:46Z

@afonso360 thanks.

github-actions · 2023-05-09T23:44:59Z

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

cfallin: isle
fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

alexcrichton · 2023-10-23T15:18:17Z

Apologies I didn't see this before I ended up making #7327, but I believe that that PR covers this one so I've annotated that to close this when merged.

yuyang-ok added 5 commits March 14, 2023 13:00

should normalize value before convert float to int on small types.

2faca4e

fix test failure

2136075

fix rounding mode.

f5e9c99

rounding mode

04dada7

better format

735408d

github-actions bot added the cranelift Issues related to the Cranelift code generator label Mar 14, 2023

disable x64 target

948d0b5

afonso360 reviewed Mar 14, 2023

View reviewed changes

cranelift/codegen/src/isa/riscv64/lower/isle.rs Outdated Show resolved Hide resolved

tests/spec_testsuite Outdated Show resolved Hide resolved

cranelift/codegen/src/isa/riscv64/inst.isle Outdated Show resolved Hide resolved

yuyang-ok added 3 commits March 14, 2023 18:01

rename and revert spec_testsuite

632b4a1

rename

3b35716

rewrite clamp value that convert from float to int with small types i…

fb919ce

…n ISLE.

fix test failure

a1525b9

afonso360 reviewed Mar 15, 2023

View reviewed changes

jameysharp reviewed Mar 15, 2023

View reviewed changes

yuyang-ok and others added 5 commits March 16, 2023 09:26

Update cranelift/codegen/src/isa/riscv64/inst.isle

69c4c58

Co-authored-by: Afonso Bordado <afonso360@users.noreply.github.com>

Update cranelift/codegen/src/isa/riscv64/inst.isle

f709c18

Co-authored-by: Jamey Sharp <jamey@minilop.net>

fix register type

0d846a1

Merge branch 'issue5992' of https://github.com/yuyang-ok/wasmtime int…

60ab386

…o issue5992

clamp value

2807c7d

yuyang-ok requested a review from a team as a code owner March 30, 2023 04:12

yuyang-ok requested review from jameysharp and removed request for a team March 30, 2023 04:12

yuyang-ok added 3 commits March 30, 2023 12:19

fix const error

3a172f7

fix

3814d2a

Merge branch 'main' of https://github.com/bytecodealliance/wasmtime i…

6459a49

…nto issue5992

fix name

5e58d36

Merge branch 'main' of https://github.com/bytecodealliance/wasmtime i…

b6c717b

…nto issue5992

github-actions bot added the isle Related to the ISLE domain-specific language label May 9, 2023

merge upstream

2fc567a

alexcrichton mentioned this pull request Oct 23, 2023

riscv64: Refactor FRM and fcvt-to-int management #7327

Merged

afonso360 closed this in #7327 Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Codegen fix convert float to int on small types with riscv64. #6015

Codegen fix convert float to int on small types with riscv64. #6015

yuyang-ok commented Mar 14, 2023 •

edited

Loading

yuyang-ok commented Mar 14, 2023

yuyang-ok commented Mar 15, 2023 •

edited

Loading

afonso360 left a comment •

edited

Loading

afonso360 Mar 15, 2023

afonso360 Mar 15, 2023

jameysharp Mar 15, 2023

afonso360 Mar 15, 2023

jameysharp Mar 15, 2023

afonso360 Mar 16, 2023

jameysharp left a comment

jameysharp Mar 15, 2023

jameysharp Mar 15, 2023

yuyang-ok commented Apr 22, 2023

afonso360 commented Apr 23, 2023

yuyang-ok commented Apr 24, 2023

github-actions bot commented May 9, 2023

alexcrichton commented Oct 23, 2023

Codegen fix convert float to int on small types with riscv64. #6015

Codegen fix convert float to int on small types with riscv64. #6015

Conversation

yuyang-ok commented Mar 14, 2023 • edited Loading

yuyang-ok commented Mar 14, 2023

yuyang-ok commented Mar 15, 2023 • edited Loading

afonso360 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameysharp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuyang-ok commented Apr 22, 2023

afonso360 commented Apr 23, 2023

yuyang-ok commented Apr 24, 2023

github-actions bot commented May 9, 2023

Subscribe to Label Action

alexcrichton commented Oct 23, 2023

yuyang-ok commented Mar 14, 2023 •

edited

Loading

yuyang-ok commented Mar 15, 2023 •

edited

Loading

afonso360 left a comment •

edited

Loading