Bug 63757 - Add optimised paths for a few maths functions
Summary: Add optimised paths for a few maths functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on: 63790
Blocks: 63893
  Show dependency treegraph
 
Reported: 2011-06-30 15:22 PDT by Oliver Hunt
Modified: 2011-07-04 01:38 PDT (History)
4 users (show)

See Also:


Attachments
Patch (23.30 KB, patch)
2011-06-30 15:26 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Patch (22.22 KB, patch)
2011-06-30 15:50 PDT, Oliver Hunt
barraclough: review+
Details | Formatted Diff | Diff
crash log on Qt in debug mode (6.88 KB, text/plain)
2011-07-01 00:45 PDT, Csaba Osztrogonác
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2011-06-30 15:22:30 PDT
Add optimised paths for a few maths functions
Comment 1 Oliver Hunt 2011-06-30 15:26:09 PDT
Created attachment 99375 [details]
Patch
Comment 2 WebKit Review Bot 2011-06-30 15:29:17 PDT
Attachment 99375 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/X86Assembler.h:1461:  andnpd_rr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/jit/SpecializedThunkJIT.h:142:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/jit/ThunkGenerators.cpp:146:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/ThunkGenerators.cpp:164:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/ThunkGenerators.cpp:169:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Gavin Barraclough 2011-06-30 15:43:17 PDT
Comment on attachment 99375 [details]
Patch

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

> Source/JavaScriptCore/jit/JSInterfaceJIT.h:327
> +    inline JSInterfaceJIT::Jump JSInterfaceJIT::emitJumpIfNotDouble(int virtualRegisterIndex, RegisterID base)

This is incorrect & unused, please remove.

> Source/JavaScriptCore/jit/SpecializedThunkJIT.h:74
> +        Jump emitJumpIfArgumentNotDouble(int argument)

This is unused, please remove.

> Source/JavaScriptCore/jit/SpecializedThunkJIT.h:143
> +        void callDoubleToDouble(double(*function)(double))

My understanding is that these will not obey this calling convention; please make this a void* / FunctionPtr / etc.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:173
> +defineD2DWrapper(jsRound);

"D2D" seems to be more of an abbreviation than our coding standard really agrees with, could we make this slightly verbier & clearer? - D2D -> UnaryMathOp?
Comment 4 Early Warning System Bot 2011-06-30 15:43:21 PDT
Comment on attachment 99375 [details]
Patch

Attachment 99375 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8957888
Comment 5 Zoltan Herczeg 2011-06-30 15:46:43 PDT
I think ARM could do these in a different way... For example it has a floating point abs function. Am I see right that jit.callDoubleToDouble(D2DWrapper(...)) could destroy registers? Hm, ARM has hardfp and softfp ABIs, makes thing a little more difficult. Anyway, how much gain on SunSpider?
Comment 6 Oliver Hunt 2011-06-30 15:50:45 PDT
Created attachment 99381 [details]
Patch
Comment 7 WebKit Review Bot 2011-06-30 15:52:39 PDT
Attachment 99381 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/assembler/X86Assembler.h:1461:  andnpd_rr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/JavaScriptCore/jit/SpecializedThunkJIT.h:136:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/JavaScriptCore/jit/ThunkGenerators.cpp:122:  Missing space inside { }.  [whitespace/braces] [5]
Source/JavaScriptCore/jit/ThunkGenerators.cpp:146:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/ThunkGenerators.cpp:164:  Extra space before ( in function call  [whitespace/parens] [4]
Source/JavaScriptCore/jit/ThunkGenerators.cpp:169:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 6 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Oliver Hunt 2011-06-30 15:54:22 PDT
(In reply to comment #5)
> I think ARM could do these in a different way... For example it has a floating point abs function. Am I see right that jit.callDoubleToDouble(D2DWrapper(...)) could destroy registers? Hm, ARM has hardfp and softfp ABIs, makes thing a little more difficult. Anyway, how much gain on SunSpider?

This currently no-ops on ARM, ARMs variety of problems makes me less inclined to try and fix it right away (and efficiency isn't perfect on any platform due to those problems), happily at the point we make the call we no longer care about any of our caller save registers (our result is dependent only the result of the call).

I have hopes the DFG jit will introduce a generalised mechanism for calling more stupid calling conventions in the future, and we can reoptimise them at that point.
Comment 9 Oliver Hunt 2011-06-30 15:55:56 PDT
(In reply to comment #5)
> I think ARM could do these in a different way... For example it has a floating point abs function. Am I see right that jit.callDoubleToDouble(D2DWrapper(...)) could destroy registers? Hm, ARM has hardfp and softfp ABIs, makes thing a little more difficult. Anyway, how much gain on SunSpider?

Around 0.9% gain on sunspider, but this is mostly to deal with certain benchmarks that claim 90% Math code == real world JS.  And i was waiting for other builds so i thought i'd hack this together.
Comment 10 Gavin Barraclough 2011-06-30 16:04:41 PDT
Comment on attachment 99381 [details]
Patch

Please to be fixing the style bot's whitespace issues that are valid, and to be making the argument type a typedef to increase readability.
Comment 11 Oliver Hunt 2011-06-30 16:08:57 PDT
Committed r90177: <http://trac.webkit.org/changeset/90177>
Comment 12 Zoltan Herczeg 2011-06-30 16:11:53 PDT
> This currently no-ops on ARM, ARMs variety of problems makes me less inclined to try and fix it right away (and efficiency isn't perfect on any platform due to those problems), happily at the point we make the call we no longer care about any of our caller save registers (our result is dependent only the result of the call).
> 
> I have hopes the DFG jit will introduce a generalised mechanism for calling more stupid calling conventions in the future, and we can reoptimise them at that point.

No problem, I thought I will try to do it myself on ARM. I just don't want to add more defines, instead I would prefer some inline methods...
Comment 13 Oliver Hunt 2011-06-30 16:14:01 PDT
(In reply to comment #12)
> > This currently no-ops on ARM, ARMs variety of problems makes me less inclined to try and fix it right away (and efficiency isn't perfect on any platform due to those problems), happily at the point we make the call we no longer care about any of our caller save registers (our result is dependent only the result of the call).
> > 
> > I have hopes the DFG jit will introduce a generalised mechanism for calling more stupid calling conventions in the future, and we can reoptimise them at that point.
> 
> No problem, I thought I will try to do it myself on ARM. I just don't want to add more defines, instead I would prefer some inline methods...

inline methods don't work because you need to be able to specify calling convention details in ways that gcc doesn't let you do -- i _think_ that you should simply be able to make the arm thunk be a vmov to r0,r1 and then back again after the call.
Comment 14 Zoltan Herczeg 2011-06-30 16:19:10 PDT
Oh, it does not compile on ARM:

http://build.webkit.sed.hu/builders/ARMv5%20Linux%20Qt%20Release%20%28Build%29/builds/28644/steps/compile-webkit/logs/stdio

Have we such form?

JSC::SpecializedThunkJIT::rshift32(const JSC::ARMRegisters::RegisterID&, JSC::AbstractMacroAssembler<JSC::ARMAssembler>::TrustedImm32, const JSC::ARMRegisters::RegisterID&)

Too late, probably do that tomorrow.
Comment 15 Oliver Hunt 2011-06-30 16:20:57 PDT
(In reply to comment #14)
> Oh, it does not compile on ARM:
> 
> http://build.webkit.sed.hu/builders/ARMv5%20Linux%20Qt%20Release%20%28Build%29/builds/28644/steps/compile-webkit/logs/stdio
> 
> Have we such form?
> 
> JSC::SpecializedThunkJIT::rshift32(const JSC::ARMRegisters::RegisterID&, JSC::AbstractMacroAssembler<JSC::ARMAssembler>::TrustedImm32, const JSC::ARMRegisters::RegisterID&)
> 
> Too late, probably do that tomorrow.

Landed theoretical fix.  It's possible i've got src/dst flipped though.
Comment 16 Zoltan Herczeg 2011-06-30 16:23:56 PDT
> Landed theoretical fix.  It's possible i've got src/dst flipped though.

Ah, it is just a short form:

308	    void rshift32(RegisterID src, TrustedImm32 imm, RegisterID dest)
309	    {
310	        if (src != dest)
311	            move(src, dest);
312	        rshift32(imm, dest);
313	    }

Why is it not in MacroAssembler.h? Not looking x86 specific...
Comment 17 Zoltan Herczeg 2011-06-30 16:24:59 PDT
> Why is it not in MacroAssembler.h? Not looking x86 specific...

Hm, no. ARM can do that in one instruction. I will add that tomorrow.
Comment 18 Csaba Osztrogonác 2011-07-01 00:45:09 PDT
Created attachment 99438 [details]
crash log on Qt in debug mode

http://build.webkit.sed.hu/results/x86-32%20Linux%20Qt%20Debug/r90205%20%2816599%29/canvas/philip/tests/2d.imageData.get.order.rows-stderr.txt

canvas/philip/tests/2d.imageData.get.order.rows.html (r90205)
Comment 19 Csaba Osztrogonác 2011-07-01 00:50:02 PDT
It was rolled out (http://trac.webkit.org/changeset/90215), because it made 
130 jscrore tests and 361 layout tests crash on Qt in debug mode:
http://build.webkit.sed.hu/builders/x86-32%20Linux%20Qt%20Debug/builds/16591

Zoltán and/or Gábor, could you help the guys debug this regression?
Comment 20 Csaba Osztrogonác 2011-07-01 02:31:42 PDT
These tests crashed on the GTK debug bot too:
http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Debug/builds/16140
Comment 21 Oliver Hunt 2011-07-01 09:34:21 PDT
Committed r90237: <http://trac.webkit.org/changeset/90237>
Comment 22 Oliver Hunt 2011-07-01 09:36:34 PDT
Everything should be fine now as i've made it mac only, i'll add windows support at some point, but from here on out i'm going to explicitly exclude anything other than max from optimisations i make.  I'm tired of making good faith efforts to fix platforms that i don't maintain being met with complete rollouts without any attempt to diagnose or fix the problem by any of the platforms maintainers.
Comment 23 Zoltan Herczeg 2011-07-01 10:01:16 PDT
(In reply to comment #22)
> Everything should be fine now as i've made it mac only, i'll add windows support at some point, but from here on out i'm going to explicitly exclude anything other than max from optimisations i make.  I'm tired of making good faith efforts to fix platforms that i don't maintain being met with complete rollouts without any attempt to diagnose or fix the problem by any of the platforms maintainers.

I understand you, and you probably understand Ossy as well. Everyone do their best effort. I think the best common denominator would be to make mac only patches, and CC'ing other platform maintainers to make the support of their platforms. Unfortunately we are on different time zones, so we may not have time to debug non-trivial issues and add support for new features, but keeping the bots green is essential since other real bugs may appear later, and we will not know about them because of red bots.