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.
Created attachment 334370 [details] Patch v2
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.
Created attachment 335179 [details] Patch
Also reproducible on ARM64, with GCC compilers: https://bugs.webkit.org/show_bug.cgi?id=179024
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?
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 on attachment 335179 [details] Patch It's not correct to fix specification failures by pretending that it's not a big deal.
(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.
(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.
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?