Bug 232951 – Prevent fused multiply add during ParseInt

• Component: New Bugs
• Status: RESOLVED FIXED

[Mikhail R. Gadelha]

Prevent fused multiply add during ParseInt

[Mikhail R. Gadelha]

Created attachment 443832 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 443833 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 443833 [details]
Patch

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

Commented.

> Source/JavaScriptCore/ChangeLog:18
> +        When parsing the string in parseInt, a compiler can wrongfully generate
> +        a fused multiply-add instruction, causing the conversion to be wrong
> +        for some high values. An add followed by a multiply gives the correct
> +        result and it is the code generated most of the times.
> +
> +        This patch adds a volatile qualifier to the number variable, so the
> +        compiler doesn't try to optimize it, and enables a failing test on
> +        mips.
> +
> +        The issue was found when cross compiling to mips with gcc 8.4.0 and
> +        options -ffp-contract=off -mmadd4.

This is GCC issue, and we should not use `volatile` here. We should disable GCC's this optimization for this function via pragma etc.
https://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/Function-Specific-Option-Pragmas.html#Function-Specific-Option-Pragmas
https://stackoverflow.com/questions/34436233/fused-multiply-add-and-default-rounding-modes

[Mikhail R. Gadelha]

(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 443833 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443833&action=review
> 
> Commented.
> 
> > Source/JavaScriptCore/ChangeLog:18
> > +        When parsing the string in parseInt, a compiler can wrongfully generate
> > +        a fused multiply-add instruction, causing the conversion to be wrong
> > +        for some high values. An add followed by a multiply gives the correct
> > +        result and it is the code generated most of the times.
> > +
> > +        This patch adds a volatile qualifier to the number variable, so the
> > +        compiler doesn't try to optimize it, and enables a failing test on
> > +        mips.
> > +
> > +        The issue was found when cross compiling to mips with gcc 8.4.0 and
> > +        options -ffp-contract=off -mmadd4.
> 
> This is GCC issue, and we should not use `volatile` here. We should disable
> GCC's this optimization for this function via pragma etc.
> https://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/Function-Specific-Option-
> Pragmas.html#Function-Specific-Option-Pragmas
> https://stackoverflow.com/questions/34436233/fused-multiply-add-and-default-
> rounding-modes

But these pragmas disable optimization for the whole function. Are you sure we should disable optimization for the whole function? I also tried to use `#pragma STDC FP_CONTRACT` but gcc doesn't seem to accept it yet: https://stackoverflow.com/questions/43352510/difference-in-gcc-ffp-contract-options

On other platforms that I tested, the compilers already generate an unfused multiply-add, so we don't seem to be generating worse code either.

[Yusuke Suzuki]

(In reply to Mikhail R. Gadelha from comment #4)
> (In reply to Yusuke Suzuki from comment #3)
> > Comment on attachment 443833 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=443833&action=review
> > 
> > Commented.
> > 
> > > Source/JavaScriptCore/ChangeLog:18
> > > +        When parsing the string in parseInt, a compiler can wrongfully generate
> > > +        a fused multiply-add instruction, causing the conversion to be wrong
> > > +        for some high values. An add followed by a multiply gives the correct
> > > +        result and it is the code generated most of the times.
> > > +
> > > +        This patch adds a volatile qualifier to the number variable, so the
> > > +        compiler doesn't try to optimize it, and enables a failing test on
> > > +        mips.
> > > +
> > > +        The issue was found when cross compiling to mips with gcc 8.4.0 and
> > > +        options -ffp-contract=off -mmadd4.
> > 
> > This is GCC issue, and we should not use `volatile` here. We should disable
> > GCC's this optimization for this function via pragma etc.
> > https://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/Function-Specific-Option-
> > Pragmas.html#Function-Specific-Option-Pragmas
> > https://stackoverflow.com/questions/34436233/fused-multiply-add-and-default-
> > rounding-modes
> 
> But these pragmas disable optimization for the whole function. Are you sure
> we should disable optimization for the whole function?

Yes, we should disable fused multiply add in this function.
Using `optimize` function attribute can enforce fp-contract option to this function, I think.

> I also tried to use
> `#pragma STDC FP_CONTRACT` but gcc doesn't seem to accept it yet:
> https://stackoverflow.com/questions/43352510/difference-in-gcc-ffp-contract-
> options
> 
> On other platforms that I tested, the compilers already generate an unfused
> multiply-add, so we don't seem to be generating worse code either.

[Mikhail R. Gadelha]

Created attachment 443839 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 443839 [details]
Patch

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

> Source/JavaScriptCore/runtime/ParseInt.h:113
> +// Due to a GCC bug found in v8.4.0, a wrong fused multiply-add optimization can be inserted when calculating the final number,
> +// in number *= radix; number += digit;, so optimizations are disabled for this functions.
> +#pragma GCC push_options
> +#pragma GCC optimize ("O0")

1. Why not only disabling fp-contract? We do not need to disable all optimizations. (See comment https://bugs.webkit.org/show_bug.cgi?id=232951#c5)
2. Why not using function attribute?
3. Use it only for GCC. Clang does not need this.

[Mikhail R. Gadelha]

(In reply to Yusuke Suzuki from comment #7)
> Comment on attachment 443839 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443839&action=review
> 
> > Source/JavaScriptCore/runtime/ParseInt.h:113
> > +// Due to a GCC bug found in v8.4.0, a wrong fused multiply-add optimization can be inserted when calculating the final number,
> > +// in number *= radix; number += digit;, so optimizations are disabled for this functions.
> > +#pragma GCC push_options
> > +#pragma GCC optimize ("O0")
> 
> 1. Why not only disabling fp-contract? We do not need to disable all
> optimizations. (See comment
> https://bugs.webkit.org/show_bug.cgi?id=232951#c5)
> 2. Why not using function attribute?

I just tried adding `__attribute__((optimize("fp-contract=off")))` locally to the function definition and gcc seems to ignore it, i.e., it generates de fma and the test fails.

> 3. Use it only for GCC. Clang does not need this.

[Yusuke Suzuki]

Comment on attachment 443839 [details]
Patch

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

>>> Source/JavaScriptCore/runtime/ParseInt.h:113
>>> +#pragma GCC optimize ("O0")
>> 
>> 1. Why not only disabling fp-contract? We do not need to disable all optimizations. (See comment https://bugs.webkit.org/show_bug.cgi?id=232951#c5)
>> 2. Why not using function attribute?
>> 3. Use it only for GCC. Clang does not need this.
> 
> I just tried adding `__attribute__((optimize("fp-contract=off")))` locally to the function definition and gcc seems to ignore it, i.e., it generates de fma and the test fails.

Did you try `GCC optimize` pragma and "fp-contract=off" combination?

[Mikhail R. Gadelha]

(In reply to Yusuke Suzuki from comment #9)
> Comment on attachment 443839 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=443839&action=review
> 
> >>> Source/JavaScriptCore/runtime/ParseInt.h:113
> >>> +#pragma GCC optimize ("O0")
> >> 
> >> 1. Why not only disabling fp-contract? We do not need to disable all optimizations. (See comment https://bugs.webkit.org/show_bug.cgi?id=232951#c5)
> >> 2. Why not using function attribute?
> >> 3. Use it only for GCC. Clang does not need this.
> > 
> > I just tried adding `__attribute__((optimize("fp-contract=off")))` locally to the function definition and gcc seems to ignore it, i.e., it generates de fma and the test fails.
> 
> Did you try `GCC optimize` pragma and "fp-contract=off" combination?

yep, I'm passing `-ffp-contract=off` in the command line and tried it with both `__attribute__((optimize("fp-contract=off")))` and `__attribute__((optimize("-ffp-contract=off")))`. Neither worked.

Maybe it's not working because of the inline attribute? gcc seems to be gracefully ignoring the attribute.

I only got it to work it either the `volatile` or `__attribute__((optimize("O0")))`.

[Yusuke Suzuki]

Comment on attachment 443839 [details]
Patch

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

>>>>> Source/JavaScriptCore/runtime/ParseInt.h:113
>>>>> +#pragma GCC optimize ("O0")
>>>> 
>>>> 1. Why not only disabling fp-contract? We do not need to disable all optimizations. (See comment https://bugs.webkit.org/show_bug.cgi?id=232951#c5)
>>>> 2. Why not using function attribute?
>>>> 3. Use it only for GCC. Clang does not need this.
>>> 
>>> I just tried adding `__attribute__((optimize("fp-contract=off")))` locally to the function definition and gcc seems to ignore it, i.e., it generates de fma and the test fails.
>> 
>> Did you try `GCC optimize` pragma and "fp-contract=off" combination?
> 
> yep, I'm passing `-ffp-contract=off` in the command line and tried it with both `__attribute__((optimize("fp-contract=off")))` and `__attribute__((optimize("-ffp-contract=off")))`. Neither worked.
> 
> Maybe it's not working because of the inline attribute? gcc seems to be gracefully ignoring the attribute.
> 
> I only got it to work it either the `volatile` or `__attribute__((optimize("O0")))`.

OK, so at this point, I think the best way is annotating this double with `volatile` *only when it is GCC*. Can you change it? And can you describe the above reasoning in the ChangeLog? (e.g. optimize attribute didn't work etc.).

[Mikhail R. Gadelha]

Created attachment 444060 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 444136 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 444136 [details]
Patch

r=me

[EWS]

Committed r285788 (244232@main): <https://commits.webkit.org/244232@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444136 [details].

[Radar WebKit Bug Importer]

<rdar://problem/85384001>