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
Patch (22.22 KB, patch)
2011-06-30 15:50 PDT, Oliver Hunt
barraclough: review+
crash log on Qt in debug mode (6.88 KB, text/plain)
2011-07-01 00:45 PDT, Csaba Osztrogonác
no flags
Oliver Hunt
Comment 1 2011-06-30 15:26:09 PDT
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
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
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
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
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
Oliver Hunt
Comment 21 2011-07-01 09:34:21 PDT
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.