Add optimised paths for a few maths functions
Created attachment 99375 [details] Patch
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 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 on attachment 99375 [details] Patch Attachment 99375 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8957888
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?
Created attachment 99381 [details] Patch
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.
(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.
(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 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.
Committed r90177: <http://trac.webkit.org/changeset/90177>
> 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...
(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.
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.
(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.
> 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...
> 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.
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)
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?
These tests crashed on the GTK debug bot too: http://build.webkit.org/builders/GTK%20Linux%2032-bit%20Debug/builds/16140
Committed r90237: <http://trac.webkit.org/changeset/90237>
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.
(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.