Bug 63757 – Add optimised paths for a few maths functions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Add optimised paths for a few maths functions

[Oliver Hunt]

Created attachment 99375 [details]
Patch

[WebKit Review Bot]

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.

[Gavin Barraclough]

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?

[Early Warning System Bot]

Comment on attachment 99375 [details]
Patch

Attachment 99375 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8957888

[Zoltan Herczeg]

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?

[Oliver Hunt]

Created attachment 99381 [details]
Patch

[WebKit Review Bot]

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.

[Oliver Hunt]

(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.

[Oliver Hunt]

(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.

[Gavin Barraclough]

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.

[Oliver Hunt]

Committed r90177: <http://trac.webkit.org/changeset/90177>

[Zoltan Herczeg]

> 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...

[Oliver Hunt]

(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.

[Zoltan Herczeg]

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.

[Oliver Hunt]

(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.

[Zoltan Herczeg]

> 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...

[Zoltan Herczeg]

> 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.

[Csaba Osztrogonác]

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)

[Csaba Osztrogonác]

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?

[Csaba Osztrogonác]

These tests crashed on the GTK debug bot too:
http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Debug/builds/16140

[Oliver Hunt]

Committed r90237: <http://trac.webkit.org/changeset/90237>

[Oliver Hunt]

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.

[Zoltan Herczeg]

(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.