WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
63757
Add optimised paths for a few maths functions
https://bugs.webkit.org/show_bug.cgi?id=63757
Summary
Add optimised paths for a few maths functions
Oliver Hunt
Reported
2011-06-30 15:22:30 PDT
Add optimised paths for a few maths functions
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Oliver Hunt
Comment 1
2011-06-30 15:26:09 PDT
Created
attachment 99375
[details]
Patch
WebKit Review Bot
Comment 2
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.
Gavin Barraclough
Comment 3
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?
Early Warning System Bot
Comment 4
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
Zoltan Herczeg
Comment 5
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?
Oliver Hunt
Comment 6
2011-06-30 15:50:45 PDT
Created
attachment 99381
[details]
Patch
WebKit Review Bot
Comment 7
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.
Oliver Hunt
Comment 8
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.
Oliver Hunt
Comment 9
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.
Gavin Barraclough
Comment 10
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.
Oliver Hunt
Comment 11
2011-06-30 16:08:57 PDT
Committed
r90177
: <
http://trac.webkit.org/changeset/90177
>
Zoltan Herczeg
Comment 12
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...
Oliver Hunt
Comment 13
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.
Zoltan Herczeg
Comment 14
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.
Oliver Hunt
Comment 15
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.
Zoltan Herczeg
Comment 16
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...
Zoltan Herczeg
Comment 17
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.
Csaba Osztrogonác
Comment 18
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
)
Csaba Osztrogonác
Comment 19
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?
Csaba Osztrogonác
Comment 20
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
Oliver Hunt
Comment 21
2011-07-01 09:34:21 PDT
Committed
r90237
: <
http://trac.webkit.org/changeset/90237
>
Oliver Hunt
Comment 22
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.
Zoltan Herczeg
Comment 23
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug