Stream: git-wasmtime

Topic: wasmtime / PR #6015 Codegen fix convert float to int on s...

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 05:20):

yuyang-ok opened PR #6015 from issue5992 to main:

This will fix #5992 and #5993

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 05:21):

yuyang-ok edited PR #6015 from issue5992 to main:

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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 05:25):

yuyang-ok updated PR #6015 from issue5992 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 05:27):

yuyang-ok edited PR #6015 from issue5992 to main:

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 support it.

I also changed the rounding mode.
<img width="929" alt="image" src="https://user-images.githubusercontent.com/96557710/224904674-f4de4893-f04a-49bc-b966-da0356e2aafc.png">

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 05:28):

yuyang-ok edited PR #6015 from issue5992 to main:

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 support it.

I also changed the rounding mode.
<img width="929" alt="image" src="https://user-images.githubusercontent.com/96557710/224904674-f4de4893-f04a-49bc-b966-da0356e2aafc.png">

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 05:29):

yuyang-ok edited PR #6015 from issue5992 to main:

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 support it.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 05:30):

yuyang-ok updated PR #6015 from issue5992 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 05:32):

yuyang-ok edited PR #6015 from issue5992 to main:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 05:35):

yuyang-ok updated PR #6015 from issue5992 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 05:37):

yuyang-ok updated PR #6015 from issue5992 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 06:38):

yuyang-ok updated PR #6015 from issue5992 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 09:22):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 09:22):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 09:22):

afonso360 created PR review comment:

What do you think about moving this to ISLE? This seems like the type of code that would be really neatly written there.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 09:22):

afonso360 created PR review comment:

I think this might have been included accidentally

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 09:22):

afonso360 created PR review comment:

Can we rename this to something like clamp_*? Unlike the normalize functions this one actually changes the value of the register.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 10:01):

yuyang-ok updated PR #6015 from issue5992 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 14 2023 at 10:05):

yuyang-ok updated PR #6015 from issue5992 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 03:12):

yuyang-ok updated PR #6015 from issue5992 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 03:26):

yuyang-ok updated PR #6015 from issue5992 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 16:50):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 16:51):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 16:51):

afonso360 created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 16:51):

afonso360 created PR review comment:

I don't think we have an established coding style for isle, but we should probably try to keep it somewhat consistent. This would usually be something along these lines in the other backends:

   (let ((min_value i32 (i8_i16_min_value ty is_signed))
         (min_reg Reg (imm in_ty (i32_2_float_bits min_value in_ty)))
         (tmp Reg (fmax_ in_ty min_reg r))
         (max_value i32 (i8_i16_max_value ty is_signed))
         (max_reg Reg (imm in_ty (i32_2_float_bits max_value in_ty))))
     (fmin_ in_ty tmp max_reg)))

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 16:51):

afonso360 created PR review comment:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 16:51):

afonso360 created PR review comment:

Shouldn't the type on fpu_rrr be $F32 as well?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 16:51):

afonso360 created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 17:02):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 19:36):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 19:36):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 22:30):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 22:30):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 22:30):

jameysharp created PR review comment:

This can be expressed more directly:

(rule (clamp_fcvt_from_float _ _ r _) r)
(rule 1 (clamp_fcvt_from_float in_ty (fits_in_16 ty) r is_signed)

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 22:30):

jameysharp created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 15 2023 at 22:30):

jameysharp created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 16 2023 at 01:26):

yuyang-ok updated PR #6015 from issue5992 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 16 2023 at 01:26):

yuyang-ok updated PR #6015 from issue5992 to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 16 2023 at 01:33):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 16 2023 at 01:33):

afonso360 created PR review comment:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 30 2023 at 04:12):

yuyang-ok requested jameysharp for a review on PR #6015.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 30 2023 at 04:12):

yuyang-ok requested wasmtime-compiler-reviewers for a review on PR #6015.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 30 2023 at 04:12):

yuyang-ok updated PR #6015.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 30 2023 at 04:19):

yuyang-ok updated PR #6015.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 23 2023 at 22:46):

yuyang-ok updated PR #6015.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 12 2023 at 22:47):

yuyang-ok updated PR #6015.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 23 2023 at 21:11):

afonso360 closed without merge PR #6015.

Last updated: Nov 22 2024 at 16:03 UTC