RESOLVED FIXED101909
Patching of jumps to stubs should use jump replacement rather than branch destination overwrite 
https://bugs.webkit.org/show_bug.cgi?id=101909
Summary Patching of jumps to stubs should use jump replacement rather than branch des...
Filip Pizlo
Reported 2012-11-12 00:37:08 PST
That will reduce the amount of code executed on the hot path.
Attachments
work in progress (12.17 KB, patch)
2012-11-12 00:40 PST, Filip Pizlo 		no flags
DetailsFormatted DiffDiff
the patch (17.60 KB, patch)
2012-11-12 16:34 PST, Filip Pizlo 		ggaren: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 2012-11-12 00:40:39 PST
Created attachment 173580 [details] work in progress
Filip Pizlo
Comment 2 2012-11-12 16:34:40 PST
Created attachment 173757 [details] the patch
Geoffrey Garen
Comment 3 2012-11-12 16:38:51 PST
Comment on attachment 173757 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=173757&action=review r=me > Source/JavaScriptCore/assembler/X86Assembler.h:1922 > +#if CPU(X86_64) > + static void revertJumpTo_movq_i64r(void* instructionStart, int64_t imm, RegisterID dst) > + { > + const int rexBytes = 1; > + const int opcodeBytes = 1; > + ASSERT(rexBytes + opcodeBytes <= maxJumpReplacementSize()); > + uint8_t* ptr = reinterpret_cast<uint8_t*>(instructionStart); > + ptr[0] = PRE_REX | (1 << 3) | (dst >> 3); > + ptr[1] = OP_MOV_EAXIv | (dst & 7); > + > + union { > + uint64_t asWord; > + uint8_t asBytes[8]; > + } u; > + u.asWord = imm; > + for (unsigned i = rexBytes + opcodeBytes; i < static_cast<unsigned>(maxJumpReplacementSize()); ++i) > + ptr[i] = u.asBytes[i - rexBytes - opcodeBytes]; > + } > +#endif > + > + static void revertJumpTo_cmpl_im_force32(void* instructionStart, int32_t imm, int offset, RegisterID dst) > + { > + ASSERT_UNUSED(offset, !offset); > + const int opcodeBytes = 1; > + const int modRMBytes = 1; > + ASSERT(opcodeBytes + modRMBytes <= maxJumpReplacementSize()); > + uint8_t* ptr = reinterpret_cast<uint8_t*>(instructionStart); > + ptr[0] = OP_GROUP1_EvIz; > + ptr[1] = (X86InstructionFormatter::ModRmMemoryNoDisp << 6) | (GROUP1_OP_CMP << 3) | dst; > + union { > + uint32_t asWord; > + uint8_t asBytes[4]; > + } u; > + u.asWord = imm; > + for (unsigned i = opcodeBytes + modRMBytes; i < static_cast<unsigned>(maxJumpReplacementSize()); ++i) > + ptr[i] = u.asBytes[i - opcodeBytes - modRMBytes]; > + } This would be a good thing to run by Gavin when he's back.
Filip Pizlo
Comment 4 2012-11-12 17:56:36 PST
Landed in http://trac.webkit.org/changeset/134332
Csaba Osztrogonác
Comment 5 2012-11-12 21:51:34 PST
(In reply to comment #4) > Landed in http://trac.webkit.org/changeset/134332 It broke the ARM traditional build - https://bugs.webkit.org/show_bug.cgi?id=102044
Csaba Osztrogonác
Comment 6 2012-11-14 06:53:51 PST
(In reply to comment #5) > (In reply to comment #4) > > Landed in http://trac.webkit.org/changeset/134332 > > It broke the ARM traditional build - https://bugs.webkit.org/show_bug.cgi?id=102044 and the MIPS build too - https://bugs.webkit.org/show_bug.cgi?id=102227
Note You need to log in before you can comment on or make changes to this bug.