Support more paren expressions in RegExp-JIT on ARM/MIPS
Created attachment 365554 [details] Patch
Attachment 365554 [details] did not pass style-queue: ERROR: Source/WTF/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:8: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:9: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:10: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:12: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:13: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:14: Line contains tab character. [whitespace/tab] [5] ERROR: Source/JavaScriptCore/ChangeLog:16: Line contains tab character. [whitespace/tab] [5] Total errors found: 8 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 365555 [details] Patch
Comment on attachment 365555 [details] Patch LGTM
Created attachment 365682 [details] Patch
Comment on attachment 365682 [details] Patch Attachment 365682 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11610448 New failing tests: http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html
Created attachment 365696 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 365682 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365682&action=review Did not test it, but the changes make sense to me, with a few nitpicks. > Source/JavaScriptCore/assembler/MacroAssembler.h:585 > + void addPtr(RegisterID src, Address address) should address be called dest instead? > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:214 > + void add32(RegisterID src, Address address) Same question as for addPtr() > Source/JavaScriptCore/yarr/YarrJIT.cpp:3785 > + pushPair(ARMRegisters::r11, ARMRegisters::r4); // push r4 twice for stack alignment Would it cost more to do something like this? (for clarity sake): push(ArmRegisters::r11); sub32(stackPointerRegister, 4); > Source/JavaScriptCore/yarr/YarrJIT.cpp:3799 > + move(TrustedImm32(0x10000), supplementaryPlanesBase); > + move(TrustedImm32(0xfffffc00), surrogateTagMask); > + move(TrustedImm32(0xd800), leadingSurrogateTag); > + move(TrustedImm32(0xdc00), trailingSurrogateTag); shouldn't we have const variables or defines for these immediate values? > Source/JavaScriptCore/yarr/YarrJIT.cpp:3851 > + popPair(ARMRegisters::r11, ARMRegisters::r4); // pop r4 twice for stack alignment see my comment when doing the matching pushPair(). > Source/WTF/wtf/Platform.h:1013 > +#if CPU(ARM64) || (CPU(X86_64) && !OS(WINDOWS)) || OS(LINUX) && (CPU(ARM_THUMB2) || CPU(MIPS)) maybe an added pair of parentheses could make the meaning more obvious: #if CPU(ARM64) || (CPU(X86_64) && !OS(WINDOWS)) || (OS(LINUX) && (CPU(ARM_THUMB2) || CPU(MIPS)))
Created attachment 365746 [details] Patch
Created attachment 365748 [details] Patch
(In reply to Guillaume Emont from comment #8) > Comment on attachment 365682 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=365682&action=review > > Did not test it, but the changes make sense to me, with a few nitpicks. > > > Source/JavaScriptCore/assembler/MacroAssembler.h:585 > > + void addPtr(RegisterID src, Address address) > > should address be called dest instead? > > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:214 > > + void add32(RegisterID src, Address address) > > Same question as for addPtr() Renamed. > > > Source/JavaScriptCore/yarr/YarrJIT.cpp:3785 > > + pushPair(ARMRegisters::r11, ARMRegisters::r4); // push r4 twice for stack alignment > > Would it cost more to do something like this? (for clarity sake): > push(ArmRegisters::r11); > sub32(stackPointerRegister, 4); I used pushPair here as well to be more in-line with the code before it. > > > Source/JavaScriptCore/yarr/YarrJIT.cpp:3799 > > + move(TrustedImm32(0x10000), supplementaryPlanesBase); > > + move(TrustedImm32(0xfffffc00), surrogateTagMask); > > + move(TrustedImm32(0xd800), leadingSurrogateTag); > > + move(TrustedImm32(0xdc00), trailingSurrogateTag); > > shouldn't we have const variables or defines for these immediate values? Maybe. Those constants were already duplicated for x64 and Arm64, I've now duplicated them for ARM and MIPS as well. It might now make sense to add constants for them. I will leave it as-is for now and see what the reviewer thinks. > > > Source/JavaScriptCore/yarr/YarrJIT.cpp:3851 > > + popPair(ARMRegisters::r11, ARMRegisters::r4); // pop r4 twice for stack alignment > > see my comment when doing the matching pushPair(). > > > Source/WTF/wtf/Platform.h:1013 > > +#if CPU(ARM64) || (CPU(X86_64) && !OS(WINDOWS)) || OS(LINUX) && (CPU(ARM_THUMB2) || CPU(MIPS)) > > maybe an added pair of parentheses could make the meaning more obvious: > #if CPU(ARM64) || (CPU(X86_64) && !OS(WINDOWS)) || (OS(LINUX) && > (CPU(ARM_THUMB2) || CPU(MIPS))) Changed.
Created attachment 366259 [details] Patch
I've updated the patch for the changes made in https://bugs.webkit.org/show_bug.cgi?id=196296.
Comment on attachment 366259 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366259&action=review Changes still make sense to me, with the small nitpick of that typo. Again assuming that it runs and pass tests, as I did not check. > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1256 > + /* Need to use zero-extened load half-word for load16. */ Typo here: s/extened/extended/
Created attachment 366269 [details] Patch
(In reply to Guillaume Emont from comment #14) > Comment on attachment 366259 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=366259&action=review > > Changes still make sense to me, with the small nitpick of that typo. Again > assuming that it runs and pass tests, as I did not check. > > > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1256 > > + /* Need to use zero-extened load half-word for load16. */ > > Typo here: s/extened/extended/ Thanks, updated. I actually copied the description from another function and fixed the spelling there as well.
Comment on attachment 366269 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=366269&action=review Looks pretty good. Got a few suggestions. > Source/JavaScriptCore/assembler/MacroAssembler.h:588 > + > + void addPtr(RegisterID src, Address dest) > + { > + add32(src, dest); > + } We probably don't need this. See below. > Source/JavaScriptCore/assembler/MacroAssembler.h:684 > + void subPtr(TrustedImm32 imm, Address dest) > + { > + sub32(imm, dest); > + } > + We probably don't need this as well. > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:220 > + void add32(RegisterID src, Address dest) > + { > + load32(dest, dataTempRegister); > + add32(src, dataTempRegister); > + store32(dataTempRegister, dest); > + } > + We probably don't need this as well. > Source/JavaScriptCore/yarr/YarrJIT.cpp:57 > + static const RegisterID initialStart = ARMRegisters::r14; > + static const RegisterID endOfStringAddress = ARMRegisters::fp; These are the same register. Won't this mess up unwinding while in YarrJIT'ed code? I see that you are saving lr on the stack, but I wonder what a debugger or crash trace shows for a backtrace. I think you can safely eliminate a register for initialStart as it is only used for the .* wrapped optimization. > Source/JavaScriptCore/yarr/YarrJIT.cpp:269 > addPtr(TrustedImm32(parenContextSize), freelistRegister, nextParenContextPointer); > - addPtr(freelistRegister, freelistSizeRegister); > - subPtr(TrustedImm32(parenContextSize), freelistSizeRegister); > + addPtr(freelistRegister, freelistSize); > + subPtr(TrustedImm32(parenContextSize), freelistSize); What about restructuring this a little to eliminate the need for the addPtr(Reg, Address) and the subPtr(Imm, Address) macro instructions? This would replace the load, add, store, load, sub, store sequence with a load, add, sub, store. How about: Move: "RegisterID nextParenContextPointer = regT2;" above to a new block for the init loop below and then restructure the calculation of the freelist end to: { #if CPU(ARM_THUMB2) RegisterID freelistSizeRegister = regT2; loadPtr(freelistSize, freelistSizeRegister); #endif addPtr(freelistRegister, freelistSizeRegister); subPtr(TrustedImm32(parenContextSize), freelistSizeRegister); #if CPU(ARM_THUMB2) storePtr(freelistSizeRegister, freelistSize); #endif } Turn the free list initialization loop to: { RegisterID nextParenContextPointer = regT2; addPtr(TrustedImm32(parenContextSize), freelistRegister, nextParenContextPointer); ... jump(loopTop); } > Source/JavaScriptCore/yarr/YarrJIT.cpp:701 > + m_callFrameSizeInBytes = alignCallFrameSizeInBytes(m_pattern.m_body->m_callFrameSize); Let's move this member initialization to the constructor.