Bug 182788 – Test fails with fused multiply-add instructions in parseInt

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Dominik Inführ]

Created attachment 333786 [details]
Patch

This patch allows to pass multiple allowed values to verify. On ARMv7 this test fails because GCC compiles parseInt with fused multiply-add instructions. The same thing happens on x64 when compiling with e.g. -march=skylake. Using fused multiply-add seems to be allowed by the standard for this radix: https://tc39.github.io/ecma262/#sec-parseint-string-radix. Another option would be to compile JSC with "-ffp-contract=off" to disable the use of fused multiply-add. I am not sure what's your preferred way of dealing with this test failure, see this patch as an initial proposal.

[Dominik Inführ]

Created attachment 334370 [details]
Patch v2

[EWS Watchlist]

Attachment 334370 [details] did not pass style-queue:


ERROR: JSTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dominik Inführ]

Created attachment 335179 [details]
Patch

[Zan Dobersek]

Also reproducible on ARM64, with GCC compilers:
https://bugs.webkit.org/show_bug.cgi?id=179024

[Mark Lam]

Comment on attachment 335179 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=335179&action=review

> JSTests/ChakraCore/test/GlobalFunctions/ParseInt1.js:280
> +verifyOneOf(parseInt("abcdefghijklm",34), [24661871785383064000, 24661871785383067000],id++,"\" Base 34 number \"");

I don't understand how fused multiply add can possibly produce a different result from regular multiply and add.  Can you please explain?

[Dominik Inführ]

Fused multiply-add only rounds/normalizes at the end, while regular multiply/add does it after each instruction. This is only a minor difference and usually this doesn't matter but this test fails due to it.

[Filip Pizlo]

Comment on attachment 335179 [details]
Patch

It's not correct to fix specification failures by pretending that it's not a big deal.

[Filip Pizlo]

(In reply to Dominik Inführ from comment #6)
> Fused multiply-add only rounds/normalizes at the end, while regular
> multiply/add does it after each instruction. This is only a minor difference
> and usually this doesn't matter but this test fails due to it.

It's incorrect to use fused multiply/add because the ECMAScript spec defines exactly what multiply and add do, including rounding.  Fusing them violates the spec.

It's totally wrong to fix spec failures by changing the test.

[Filip Pizlo]

(In reply to Dominik Inführ from comment #0)
> Created attachment 333786 [details]
> Patch
> 
> This patch allows to pass multiple allowed values to verify. On ARMv7 this
> test fails because GCC compiles parseInt with fused multiply-add
> instructions. The same thing happens on x64 when compiling with e.g.
> -march=skylake. Using fused multiply-add seems to be allowed by the standard
> for this radix: https://tc39.github.io/ecma262/#sec-parseint-string-radix.
> Another option would be to compile JSC with "-ffp-contract=off" to disable
> the use of fused multiply-add. I am not sure what's your preferred way of
> dealing with this test failure, see this patch as an initial proposal.

JSC is supposed to be a portable VM.  I don't think we ever want to allow something as fundamental as parseInt to behave differently depending on whether multiply and add can be fused.

[Dominik Inführ]

Thanks for looking into this patch. I opened this bug to see what's your stance on this issue. If I am not mistaken the spec might allow it by stating that the result "may be an implementation-defined approximation" (https://tc39.github.io/ecma262/#sec-parseint-string-radix 18.2.5.13). So do you disable FMA for building JSC and should we do the same? What's your suggestion to fix this test failure we see?