Bug 182788 - Test fails with fused multiply-add instructions in parseInt
Summary: Test fails with fused multiply-add instructions in parseInt
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-14 04:46 PST by Dominik Inführ
Modified: 2018-05-03 07:10 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.98 KB, patch)
2018-02-14 04:46 PST, Dominik Inführ
no flags Details | Formatted Diff | Diff
Patch v2 (1.97 KB, patch)
2018-02-21 06:01 PST, Dominik Inführ
no flags Details | Formatted Diff | Diff
Patch (2.25 KB, patch)
2018-03-07 01:26 PST, Dominik Inführ
fpizlo: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Inführ 2018-02-14 04:46:23 PST
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.
Comment 1 Dominik Inführ 2018-02-21 06:01:29 PST
Created attachment 334370 [details]
Patch v2
Comment 2 Build Bot 2018-02-28 11:04:00 PST
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.
Comment 3 Dominik Inführ 2018-03-07 01:26:37 PST
Created attachment 335179 [details]
Patch
Comment 4 Zan Dobersek 2018-03-07 09:42:42 PST
Also reproducible on ARM64, with GCC compilers:
https://bugs.webkit.org/show_bug.cgi?id=179024
Comment 5 Mark Lam 2018-03-07 09:51:16 PST
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?
Comment 6 Dominik Inführ 2018-03-07 11:03:45 PST
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.
Comment 7 Filip Pizlo 2018-03-07 11:06:02 PST
Comment on attachment 335179 [details]
Patch

It's not correct to fix specification failures by pretending that it's not a big deal.
Comment 8 Filip Pizlo 2018-03-07 11:06:57 PST
(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.
Comment 9 Filip Pizlo 2018-03-07 11:07:52 PST
(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.
Comment 10 Dominik Inführ 2018-03-07 11:21:04 PST
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?