Summary: | Prevent fused multiply add during ParseInt | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mikhail R. Gadelha <mikhail> | ||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Mikhail R. Gadelha
2021-11-10 10:12:36 PST
Created attachment 443832 [details]
Patch
Created attachment 443833 [details]
Patch
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 (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. (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. Created attachment 443839 [details]
Patch
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. (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. 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? (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")))`. 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.). Created attachment 444060 [details]
Patch
Created attachment 444136 [details]
Patch
Comment on attachment 444136 [details]
Patch
r=me
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]. |