Bug 115138

Created attachment 199536 [details] Patch Side note: this can be easily extended to other systems (e.g. Linux on ARM) that need to know about hardware idiv availability at run time, by adding the appropriate checks to isIntegerDivSupported().

Created attachment 199537 [details] Patch Fixed style.

Comment on attachment 199537 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=199537&action=review Nope. > Source/JavaScriptCore/dfg/DFGCommon.h:109 > #if CPU(APPLE_ARMV7S) > return true; > +#elif CPU(ARM_THUMB2) && OS(QNX) > + return MacroAssembler::supportsIntegerDiv(); This is terribly misleading. What you should do is: -MacroAssembler::supportsIntegerDiv() is a _compile time_ constant for everything but QNX -It is true for ARMv7s, false everywhere else. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:2256 > #elif CPU(APPLE_ARMV7S) > - compileIntegerArithDivForARMv7s(node); > + compileIntegerArithDivForARM(node); > +#elif CPU(ARM_THUMB2) && OS(QNX) > + if (MacroAssembler::supportsIntegerDiv()) > + compileIntegerArithDivForARM(node); That makes no sense. What if you are ArithDiv and MacroAssembler::supportsIntegerDiv() evaluate to false, you just silently ignore the operation?

Created attachment 199607 [details] Patch (In reply to comment #3) > What you should do is: > -MacroAssembler::supportsIntegerDiv() is a _compile time_ constant for everything but QNX > [...] Done. This allowed me to simplify preprocessor conditionals in other places, and rely on the fact that this gets resolved at compile time whenever possible. > That makes no sense. What if you are ArithDiv and MacroAssembler::supportsIntegerDiv() evaluate to false, you just silently ignore the operation? From what I understood so far, the call site is responsible not to branch here if IDIV isn't available, hence the RELEASE_ASSERT_NOT_REACHED further below. And you're right, that if-statement shouldn't be there. I changed it into an assertion.

Created attachment 199614 [details] Patch I initially made DFG::isARMWithIntegerDiv() in order to use it as it's the case with DFG::isX86(), i.e. when it's not protected by #if CPU(...); but then I noticed I had used it inside in ARM-specific code also, interchangeably (and inconsistently) with MacroAssembler::supportsIntegerDiv(). I am now modifying the previous patch: I use MacroAssembler::supportsIntegerDiv() consistently inside ARM-specific code, and DFG::isARMWithIntegerDiv() only in the rare cases outside. I also simplified isIntegerDivSupported().

Comment on attachment 199614 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=199614&action=review Better but there is still some confusions with your flags. > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.cpp:49 > +namespace JSC { > + > +static bool isIntegerDivSupported() > +{ > +#if CPU(APPLE_ARMV7S) > + return true; > +#elif OS(QNX) && defined(ARM_CPU_FLAG_IDIV) > + return !!(SYSPAGE_ENTRY(cpuinfo)->flags & ARM_CPU_FLAG_IDIV); > +#else > + return false; > +#endif > +} > + > +const bool MacroAssemblerARMv7::s_isIntegerDivSupported = isIntegerDivSupported(); This whole code could be in if OS(QNX). You don't need ARMV7S and the fallback. > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1921 > + > + static const bool s_isIntegerDivSupported; This should be in #if OS(QNX)

Created attachment 199673 [details] Patch (In reply to comment #6) > This whole code could be in if OS(QNX). > You don't need ARMV7S and the fallback. Done.

I am changing the summary line to reflect that improvements are no longer limited to hardware IDIV, or to QNX. Specifically, I implemented a speedup for the software-emulated modulo operation, by replacing fmod() with div().

Created attachment 199890 [details] Patch Although I only tested this on ARM, I believe that using div() instead of fmod() will benefit virtually all the other platforms also, if the necessary changes are made inside compileSoftModulo().

Created attachment 199891 [details] Patch Fixed a line in ChangeLog.

Comment on attachment 199891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=199891&action=review Looks reasonable. Some comments bellow. I would prefer if Filip could double check before you land. > Source/JavaScriptCore/ChangeLog:38 > + (JSC::DFG::SpeculativeJIT::compileSoftModuloForX86): Added. > + (JSC::DFG::SpeculativeJIT::compileSoftModuloForARM): Added. > + (JSC::DFG::SpeculativeJIT::compileSoftModulo): Split off the X86 and ARM codegen. Why not just name them all compileSoftModulo? That would make the call site simpler. > Source/JavaScriptCore/dfg/DFGOperations.cpp:1607 > + div_t quotientAndRemainder = div(a, b); > + return quotientAndRemainder.rem; a % b?

Created attachment 200069 [details] Patch Addressing reviewer's comments.

Comment on attachment 200069 [details] Patch I like this. R=me too.

Comment on attachment 200069 [details] Patch (In reply to comment #13) > (From update of attachment 200069 [details]) > I like this. R=me too. Thanks for checking the patch. Let's cq.

Comment on attachment 200069 [details] Patch Clearing flags on attachment: 200069 Committed r149349: <http://trac.webkit.org/changeset/149349>

All reviewed patches have been landed. Closing bug.

A speculative build fix landed for Qt ARM and Mips in http://trac.webkit.org/changeset/149354.

Re-opened since this is blocked by bug 115443

Comment on attachment 199891 [details] Patch Cleared Benjamin Poulain's review+ from obsolete attachment 199891 [details] so that this bug does not appear in http://webkit.org/pending-commit.

Is there any reason why is it closed as RESOLVED/WONTFIX?