WebAssembly / relaxed-simd

Relax the strict determinism requirements of SIMD operations.
Other
43 stars 9 forks source link

Float-to-signed-integer conversion in Overview.md vs spec #126

Closed alexcrichton closed 1 year ago

alexcrichton commented 1 year ago

For the i32x4.relaxed_trunc_f32x4_s instruction, for example, the current algorithm as listed in Overview.md states:

def relaxed_i32x4_trunc_f32x4_s(a : f32x4) -> i32x4:
    result = [0, 0, 0, 0]
    for i in range(4):
      if isnan(a[i]):
        result[i] = IMPLEMENTATION_DEFINED_ONE_OF(0, INT32_MAX)
      r = truncate(a[i])
      if r < INT32_MIN:
        result[i] = IMPLEMENTATION_DEFINED_ONE_OF(INT32_MIN, INT32_MAX)
      elif r > INT32_MAX
        result[i] = INT32_MAX
      else:
        result[i] = r

but the current proposed spec from https://github.com/WebAssembly/relaxed-simd/pull/115 has:

$$$ \begin{array}{@{}llcll} \EXPROFDET & \relaxedtrunc^s{M,N}(\pm \NAN(n)) &=& [ 0, -2^{N-1} ] \ \EXPROFDET & \relaxedtrunc^s{M,N}(\pm q) &=& \truncs{M,N}(\pm q) & (\iff -2^{N-1} - 1 < \trunc(\pm q) < 2^{N-1}) \ \EXPROFDET & \relaxedtrunc^s{M,N}(\pm p) &=& [ \truncsats{M,N}(\pm p), -2^{N-1} ] & (\otherwise) \ & \relaxedtrunc^s{M,N}(\pm p) &=& \truncsats_{M,N}(\pm p) & (\otherwise) \ \end{array} $$$

(sorry not sure how best to render this more human-readable with GitHub)

This seems to have a few discrepancies I was hoping to clarify with this issue:

ngzhian commented 1 year ago

For NaN the overview says the result is 0 or MAX, but the spec says 0 or MIN. I believe that cvttps2dq, the intended x86_64 instruction according to relaxed i32x4.trunc_satf32x4{s,u} i32x4.trunc_satf64x2{s,u}_zero #21, returns MIN so I suspect this may be a typo in the overview.

typo in overview, should be INT32_MIN

For number-out-of-range the overview specifically lists a few cases whereas the spec lists the possibilities as either MIN or the saturated result. These two cases differ for less-than-the-minimum where the spec would require that to be MIN but the overview additionally allows MAX. I think cvttps2dq returns MIN in this case, so I'm wondering if this is another possibly typo in the out-of-bounds cases in the overview?

Yup, INT32_MAX shouldn't be there, it should always be INT32_MIN.

The r > INT32_MAX case should be expanded to allow [INT32_MIN, INT32_MAX].

Thanks for looking at the details and for reporting this!

ngzhian commented 1 year ago

Fixed in #127