...
Created attachment 388469 [details] WIP - Patch This patch depends on patch uploaded on https://bugs.webkit.org/show_bug.cgi?id=206603 to fix build of JIT on 32-bit ports.
Created attachment 388485 [details] Patch
Created attachment 388535 [details] Patch
Comment on attachment 388535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388535&action=review Added comment. > Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310 > +#if USE(JSVALUE64) Let's avoid adding `USE(JSVALUE64)` / `USE(JSVALUE32_64)` as much as possible when adding a new code. Use headerSizeInRegisters's number instead. And we are also using `pivot` in the code above. Why is it OK?
Comment on attachment 388535 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388535&action=review >> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310 >> +#if USE(JSVALUE64) > > Let's avoid adding `USE(JSVALUE64)` / `USE(JSVALUE32_64)` as much as possible when adding a new code. Use headerSizeInRegisters's number instead. > And we are also using `pivot` in the code above. Why is it OK? I'll update as requested, but I think current math for 32-bits is not correct yet. The usage of `pivot` before is fine because it will copy the entire CallFrame after the first 16-bytes of scratch. The problem is that on 32-bits, due to sizeof(intptr_t) == 4, the operation `bitwise_cast<intptr_t*>(pivot - 1) + currentEntry.offsetAsIndex())` will clobber the last 4-bytes of pivot[-1](that points to ReturnPC) when offsetAsIndex is 1, while it will properly place value on pivot[0] on 64-bits. What we need to do is to place callee-save registers starting at pivot[0]. I'm illustrating below how we would like to have pivot populated on each architecture. 64-bits pivot[-5] = ArgumentCountIncludingThis pivot[-4] = Callee pivot[-3] = CodeBlock pivot[-2] = ReturnPC pivot[-1] = callerFrame pivot[0] = csr0 ... On 32-bits it needs to be pivot[-4] = ArgumentCountIncludingThis pivot[-3] = Callee pivot[-2] = CodeBlock pivot[-1] = CallerFrameAndReturnPC pivot[0] = csr0/csr1 ... Current code is leaving a hole of 4-bytes on `pivot[0]`.
Created attachment 388590 [details] Patch
Comment on attachment 388590 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388590&action=review Looks good, but put r- since I found some nits. > Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310 > + if (sizeof(intptr_t) == 8) This is not correct. In ARM64_32, we are using two 8 bytes slots for callFrame and PC while `sizeof(intptr_t) == 4`. See comment in CallFrame.h. > Source/JavaScriptCore/dfg/DFGOSRExit.cpp:615 > + case InPair: This only exists for 32_64. So, let's write #if USE(JSVALUE32_64) case InPair: #endif instead of using `#else` of `#if USE(JSVALUE64)`. > Source/JavaScriptCore/dfg/DFGOSRExit.cpp:634 > +#endif > + case BooleanDisplacedInJSStack: > + case CellDisplacedInJSStack: > case DisplacedInJSStack: { > sideState->tmps[i] = reinterpret_cast<JSValue*>(tmpScratch)[i + tmpOffset]; > break; > } > +#if USE(JSVALUE32_64) > + case UnboxedCellInGPR: { > + EncodedValueDescriptor* valueDescriptor = bitwise_cast<EncodedValueDescriptor*>(tmpScratch + i + tmpOffset); > + sideState->tmps[i] = JSValue(JSValue::CellTag, valueDescriptor->asBits.payload); > + break; > + } > + > + case UnboxedBooleanInGPR: { > + sideState->tmps[i] = jsBoolean(static_cast<bool>(tmpScratch[i + tmpOffset])); > + break; > + } > +#endif Having the same entries switched by `ifdef` is hard to read. Let's write it in a simpler way, like this. case UnboxedCellInGPR: { #if USE(JSVALUE64) sideState->tmps[i] = reinterpret_cast<JSValue*>(tmpScratch)[i + tmpOffset]; #else EncodedValueDescriptor* valueDescriptor = bitwise_cast<EncodedValueDescriptor*>(tmpScratch + i + tmpOffset); sideState->tmps[i] = JSValue(JSValue::CellTag, valueDescriptor->asBits.payload); #endif break; }
Created attachment 388671 [details] Patch
Comment on attachment 388590 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388590&action=review Thank you very much for the review. >> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310 >> + if (sizeof(intptr_t) == 8) > > This is not correct. In ARM64_32, we are using two 8 bytes slots for callFrame and PC while `sizeof(intptr_t) == 4`. See comment in CallFrame.h. I see. I was not aware of this architecture until now. Do we use JIT on ARM64_32? I'm changing this condition to check CallFrame::headerSizeInSlots, but none of those operations will work properly on ARM64_32. >> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:615 >> + case InPair: > > This only exists for 32_64. So, let's write > > #if USE(JSVALUE32_64) > case InPair: > #endif > > instead of using `#else` of `#if USE(JSVALUE64)`. Done. >> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:634 >> +#endif > > Having the same entries switched by `ifdef` is hard to read. Let's write it in a simpler way, like this. > > case UnboxedCellInGPR: { > #if USE(JSVALUE64) > sideState->tmps[i] = reinterpret_cast<JSValue*>(tmpScratch)[i + tmpOffset]; > #else > EncodedValueDescriptor* valueDescriptor = bitwise_cast<EncodedValueDescriptor*>(tmpScratch + i + tmpOffset); > sideState->tmps[i] = JSValue(JSValue::CellTag, valueDescriptor->asBits.payload); > #endif > break; > } Done.
Comment on attachment 388590 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388590&action=review >>> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310 >>> + if (sizeof(intptr_t) == 8) >> >> This is not correct. In ARM64_32, we are using two 8 bytes slots for callFrame and PC while `sizeof(intptr_t) == 4`. See comment in CallFrame.h. > > I see. I was not aware of this architecture until now. Do we use JIT on ARM64_32? I'm changing this condition to check CallFrame::headerSizeInSlots, but none of those operations will work properly on ARM64_32. We are not using JIT for ARM64_32 while we are using LLInt. But we are aware of this architecture and I don't think using `sizeof(intptr_t)` here is a good idea.
Comment on attachment 388671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388671&action=review r=me with nits. > Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310 > + if (CallFrame::headerSizeInRegisters == 5) Let's use if constexpr (CallFrame::CallerFrameAndPC::sizeInRegisters == 2) { } else { } Since CallerFrameAndPC is the problematic part. > Source/JavaScriptCore/jit/GPRInfo.h:536 > + static constexpr unsigned numberOfRegisters = 9; Nice.
(In reply to Yusuke Suzuki from comment #10) > Comment on attachment 388590 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=388590&action=review > > >>> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310 > >>> + if (sizeof(intptr_t) == 8) > >> > >> This is not correct. In ARM64_32, we are using two 8 bytes slots for callFrame and PC while `sizeof(intptr_t) == 4`. See comment in CallFrame.h. > > > > I see. I was not aware of this architecture until now. Do we use JIT on ARM64_32? I'm changing this condition to check CallFrame::headerSizeInSlots, but none of those operations will work properly on ARM64_32. > > We are not using JIT for ARM64_32 while we are using LLInt. But we are aware > of this architecture and I don't think using `sizeof(intptr_t)` here is a > good idea. Thanks for the heads up and I agree. I updated the patch to use `headerSizeInRegisters` instead and placed some assertions there as well. BTW< I saw the comment in CallFrame.h before, but my understanding was that caller frame and return pc together uses 8 bytes (like it is on ARMv7 and MIPS). Maybe we should change it to `arm64_32 expects caller frame and return pc to use 8 bytes each`.
Created attachment 388706 [details] Patch
Comment on attachment 388671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388671&action=review Thanks again for the review. >> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310 >> + if (CallFrame::headerSizeInRegisters == 5) > > Let's use > > if constexpr (CallFrame::CallerFrameAndPC::sizeInRegisters == 2) { > } else { > } > > Since CallerFrameAndPC is the problematic part. Done.
Comment on attachment 388706 [details] Patch Clearing flags on attachment: 388706 Committed r255088: <https://trac.webkit.org/changeset/255088>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58877206>