|Summary:||[ARM] Expand the use of integer division|
|Product:||WebKit||Reporter:||Cosmin Truta <ctruta>|
|Severity:||Normal||CC:||benjamin, cmarcelo, commit-queue, gyuyoung.kim, ossy, rakuco, zarvai|
|Version:||528+ (Nightly build)|
|Bug Depends on:||108645, 115443, 115444|
Description Cosmin Truta 2013-04-24 16:13:39 PDT
Comment 1 Cosmin Truta 2013-04-24 16:56:44 PDT
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().
Comment 2 Cosmin Truta 2013-04-24 16:59:49 PDT
Created attachment 199537 [details] Patch Fixed style.
Comment 3 Benjamin Poulain 2013-04-24 17:11:16 PDT
Comment 4 Cosmin Truta 2013-04-24 19:13:44 PDT
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.
Comment 5 Cosmin Truta 2013-04-24 20:46:59 PDT
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 6 Benjamin Poulain 2013-04-24 22:23:41 PDT
Comment 7 Cosmin Truta 2013-04-25 09:37:56 PDT
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.
Comment 8 Cosmin Truta 2013-04-26 22:00:24 PDT
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().
Comment 9 Cosmin Truta 2013-04-26 22:05:32 PDT
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().
Comment 10 Cosmin Truta 2013-04-26 22:08:18 PDT
Created attachment 199891 [details] Patch Fixed a line in ChangeLog.
Comment 11 Benjamin Poulain 2013-04-29 17:58:15 PDT
Comment 12 Cosmin Truta 2013-04-29 18:28:20 PDT
Created attachment 200069 [details] Patch Addressing reviewer's comments.
Comment 13 Filip Pizlo 2013-04-29 18:33:39 PDT
Comment on attachment 200069 [details] Patch I like this. R=me too.
Comment 14 Benjamin Poulain 2013-04-29 19:27:59 PDT
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 15 WebKit Commit Bot 2013-04-29 19:54:19 PDT
Comment on attachment 200069 [details] Patch Clearing flags on attachment: 200069 Committed r149349: <http://trac.webkit.org/changeset/149349>
Comment 16 WebKit Commit Bot 2013-04-29 19:54:23 PDT
All reviewed patches have been landed. Closing bug.
Comment 17 Zoltan Arvai 2013-04-30 00:43:23 PDT
A speculative build fix landed for Qt ARM and Mips in http://trac.webkit.org/changeset/149354.
Comment 19 Csaba Osztrogonác 2014-02-13 04:04:36 PST
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.
Comment 20 Csaba Osztrogonác 2014-02-14 01:23:04 PST
Is there any reason why is it closed as RESOLVED/WONTFIX?