745324 - ClampDoubleToUint8 / js_TypedArray_uint8_clamp_double incorrectly rounds towards 0.

Status: NEW

(Core :: JavaScript Engine, defect)

Comment 1

[Marty Rosenberg [:mjrosenb]]

http://dev.w3.org/2006/webapi/WebIDL/#es-octet 3.1 says to round towards even when there is a tie.
When we attempt to clamp 0.50000000000000006 through 0.50000000000000017 (all represented as 0.5 with the least significant bit set), we get 0, rather than 0.  This happens because
our algorithm looks something like
double toTruncate = x + 0.5;
uint8_t y = uint8_t(toTruncate);
if (y == toTruncate) {                                                                                                return (y & ~1);
}
return y
when 0.5+\epsilon is passed in, we add 0.5 to it, which produces *exactly* 1.0, converting this to an integer and back confirms that the value is 1.0, and thus the original value "must" have been 0.5 exactly, which needs to get rounded towards the nearest even value, 0.0.

This may be a problem anyplace that round-towards-even has been implemented, I haven't checked any other place.

Comment 2

[Marty Rosenberg [:mjrosenb]]

(In reply to Marty Rosenberg [:mjrosenb] from comment #1)
> http://dev.w3.org/2006/webapi/WebIDL/#es-octet 3.1 says to round towards
> even when there is a tie.
> When we attempt to clamp 0.50000000000000006 through 0.50000000000000017
> (all represented as 0.5 with the least significant bit set), we get 0,
> rather than 0.  This happens because
> our algorithm looks something like
> double toTruncate = x + 0.5;
> uint8_t y = uint8_t(toTruncate);
> if (y == toTruncate) {                                                      
> return (y & ~1);
> }
> return y
> when 0.5+\epsilon is passed in, we add 0.5 to it, which produces *exactly*
> 1.0, converting this to an integer and back confirms that the value is 1.0,
> and thus the original value "must" have been 0.5 exactly, which needs to get
> rounded towards the nearest even value, 0.0.
> 
> This may be a problem anyplace that round-towards-even has been implemented,
> I haven't checked any other place.
This should read:
When we attempt to clamp 0.50000000000000006 through 0.50000000000000017 (all represented as 0.5 with the least significant bit set), we get 0, rather than 1<this initially read "0">.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: bug745324.patch

Here is a patch that fixes it in the interpreter. For what it's worth, the clamping algorithm is detailed in https://people.mozilla.org/~jorendorff/es6-draft.html#sec-touint8clamp (step 7 should read as: if (f + 0.5 < x) then return f + 1 -- the spec bug has been already filed by jorendorff).

Let x be the input, the spec says to do the following
if Truncate(x) + 0.5 < x, then
  return Truncate(x) (i.e. Floor(x))
else
  return Truncate(x) + 1 (i.e. Ceil(x))

What we currently do:
if Truncate(x + 0.5) == x + 0.5
  return Truncate(x + 0.5) & ~1
else
  return Truncate(x + 0.5)

The algorithms seem to do almost the same thing, although the half is inside the Truncate operation in one and outside in the other, thus the error. The algorithm as specified in ES6 spec doesn't suffer from this issue, as all numbers in [0; 255] +.5 can be represented exactly as ieee754 doubles.

The interpreter can be easily fixed. However, for Baseline / Ion, that's slightly more complicated, as more values are live: as ToDouble(Truncate(x)), 0.5 and x are needed, this would require 3 FP registers instead of the 2 we currently have. Does it seem fine to you?

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: the right patch

Forgot to qcref / revert --all / export.

Comment 5

[Marty Rosenberg [:mjrosenb]]

We should certainly follow the spec if it isn't going to be a huge pain (e.g. special casing 0.499999...)
A third scratch register shouldn't be too detrimental (on arm, we already have two float32 scratch registers!)  It looks like the code will be a bit simpler, that may offset the extra register we'll need.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Patch + test - NO ARM

This does the right thing in the x86 macro-asm clampDoubleToUint8 function. Two notes regarding this implementation:
- we can get rid of the input clobbering.
- LClampDToUint8 had a unused temp; this patch now uses it.

Marty: as I don't understand the two ARM variants, I was wondering whether you'd be interested in making the same implementation for ARM. Otherwise, I'll do it later, but it might take more time.

Jan: what do you think about the updated version of the SetElem IC? It looks fine to use FloatRegister1 as it is (apparently) unused but I want to make sure this is correct.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8424901 [details] [diff] [review]
Patch + test - NO ARM

Review of attachment 8424901 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> Jan: what do you think about the updated version of the SetElem IC? It looks
> fine to use FloatRegister1 as it is (apparently) unused but I want to make
> sure this is correct.

Yes that's correct; it's fine to clobber FloatReg1.