Using more JIT thunks.
Created attachment 429340 [details] proposed patch.
<rdar://problem/78328476>
Created attachment 429342 [details] proposed patch.
Comment on attachment 429342 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=429342&action=review r=me > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1890 > + if constexpr (thunkNotUsedForResolveScope(resolveType)) { \ > + RELEASE_ASSERT_NOT_REACHED(); \ > + return { }; \ > + } \ Why not just RELEASE_ASSERT in generateOpResolveScopeThunk > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1910 > +#if CPU(X86_64) > + jit.push(X86Registers::ebp); > +#elif CPU(ARM64) > + jit.pushPair(framePointerRegister, linkRegister); > +#endif not just emit a prologue? > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1924 > + jit.setupArguments<decltype(operationGetFromScope)>(globalObjectGPR, instructionGPR); operationResolveScopeForBaseline, not operationGetFromScope > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1933 > +#if CPU(X86_64) > + jit.pop(X86Registers::ebp); > +#elif CPU(ARM64) > + jit.popPair(CCallHelpers::framePointerRegister, CCallHelpers::linkRegister); > +#endif ditto for epilogue > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:2353 > + jit.move(metadataGPR, GPRInfo::numberTagRegister); // Preserve metadata in a callee saved register. I'd assert numberTagRegister is a CSR > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:2360 > + jit.move(TrustedImm64(JSValue::NumberTag), GPRInfo::numberTagRegister); I'd do this before the exception check for sanity
Comment on attachment 429342 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=429342&action=review Thanks for the review. >> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1890 >> + } \ > > Why not just RELEASE_ASSERT in generateOpResolveScopeThunk I will do that. >> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1910 >> +#endif > > not just emit a prologue? We can't use emitFunctionPrologue() because we don't want to modify the framePointerRegister. emitFunctionPrologue() will move the sp into fp. The thunk should preserve the frame of the JS function that called it. The only reason for pushing here is to preserve the return address because this thunk can make a call, and hence, trash the linkRegister. For both ARM64 and x86_64, we push the framePointerRegister here just to keep the stack aligned. We could have pushed any other register, but fp seems natural. >> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1924 >> + jit.setupArguments<decltype(operationGetFromScope)>(globalObjectGPR, instructionGPR); > > operationResolveScopeForBaseline, not operationGetFromScope Will fix. >> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1933 >> +#endif > > ditto for epilogue Ditto: emitFunctionEpilogue() modifies sp with fp. We also don't want to do that. >> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:2353 >> + jit.move(metadataGPR, GPRInfo::numberTagRegister); // Preserve metadata in a callee saved register. > > I'd assert numberTagRegister is a CSR Will assert. >> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:2360 >> + jit.move(TrustedImm64(JSValue::NumberTag), GPRInfo::numberTagRegister); > > I'd do this before the exception check for sanity Can't do that. Before we restore the numberTagRegister, we need to store the result in the profile. However, we only want to store the result in the profile if the result is valid because we don't want to pollute the profile with junk. In the presence of an exception, it's not valid.
I'm going to invert thunkNotUsedForGetFromScope => thunkIsUsedForGetFromScope to avoid a double negative with the new assert I have to add. Ditto for thunkNotUsedForResolveScope.
Created attachment 429390 [details] patch for landing.
The JSC EWS bot failures appear to be real. However, they only reproduce on x86_64. I will investigate.
(In reply to Mark Lam from comment #8) > The JSC EWS bot failures appear to be real. However, they only reproduce on > x86_64. I will investigate. The jsc bot failures are due to flaky test failures. I'll try to address that in another bug before landing this one.
Created attachment 429618 [details] patch for landing.
Landed in r278029: <http://trac.webkit.org/r278029>.