[Webkit-unassigned] [Bug 232079] [JSC][32bit] Use DataIC in Baseline JIT
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 21 18:23:01 PDT 2021
https://bugs.webkit.org/show_bug.cgi?id=232079
Yusuke Suzuki <ysuzuki at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #442042|review? |review-
Flags| |
--- Comment #3 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 442042
--> https://bugs.webkit.org/attachment.cgi?id=442042
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=442042&action=review
Nice. I've commented. Since it has several things I would like to get anwsers, I put r-.
> Source/JavaScriptCore/jit/AssemblyHelpers.h:910
> + Jump branchIfNotObject(JSValueRegs cellJSR)
> + {
> + return branchIfNotObject(cellJSR.payloadGPR());
> + }
branchIfNotObject only accepts cellGPR. So we should not define JSValueRegs version. Let's remove this.
> Source/JavaScriptCore/jit/GPRInfo.h:79
> + constexpr bool uses(GPRReg gpr) const { return m_gpr == gpr; }
We should ignore InvalidGPRReg.
if (gpr == InvalidGPRReg)
return false;
return m_gpr == gpr;
> Source/JavaScriptCore/jit/GPRInfo.h:214
> + constexpr bool uses(GPRReg gpr) const { return m_tagGPR == gpr || m_payloadGPR == gpr; }
We should ignore if gpr is InvalidGPRReg since 32bit JSValueReg can have payloadOnly, in which we only have payload GPR.
> Source/JavaScriptCore/jit/JIT.h:340
> + void emitArrayProfilingSiteWithCell(const Bytecode&, JSValueRegs cellJSR, RegisterID scratchGPR);
Since we only accept cell GPR, we should not use JSValueRegs here.
Let's take GPRReg cellGPR.
> Source/JavaScriptCore/jit/JIT.h:342
> + void emitArrayProfilingSiteWithCell(const Bytecode&, ptrdiff_t, JSValueRegs cellJSR, RegisterID scratchGPR);
Ditto.
> Source/JavaScriptCore/jit/JITCall.cpp:137
> + // TODO: WHY THE DIFFERENCE??
WebKit does not use TODO. Please remove it.
> Source/JavaScriptCore/jit/JITCall.cpp:478
> + using namespace BaselineGetByIdRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITCall.cpp:488
> -
> - constexpr GPRReg baseGPR = BaselineGetByIdRegisters::base;
> - constexpr GPRReg resultGPR = BaselineGetByIdRegisters::result;
> - constexpr GPRReg stubInfoGPR = BaselineGetByIdRegisters::stubInfo;
>
> - move(regT0, baseGPR);
> - emitJumpSlowCaseIfNotJSCell(baseGPR);
> + emitJumpSlowCaseIfNotJSCell(returnValueJSR);
>
> JITGetByIdGenerator gen(
> nullptr, JITType::BaselineJIT, CodeOrigin(m_bytecodeIndex), CallSiteIndex(BytecodeIndex(m_bytecodeIndex.offset())), RegisterSet::stubUnavailableRegisters(),
> - CacheableIdentifier::createFromImmortalIdentifier(ident->impl()), JSValueRegs(baseGPR), JSValueRegs(resultGPR), stubInfoGPR, AccessType::GetById);
> + CacheableIdentifier::createFromImmortalIdentifier(ident->impl()), returnValueJSR, resultJSR, stubInfoGPR, AccessType::GetById);
>
It looks like original 64bit code had baseGPR (and move was done), but it is removed. Can you recover that?
And we were using different registers for JITGetByIdGenerator's returnValueJSR, resultJSR previously.
> Source/JavaScriptCore/jit/JITCall.cpp:517
> + notObject.append(branchIfNotObject(iteratorJSR));
We should pass iteratorJSR.payloadGPR() since we already know this is a cell.
> Source/JavaScriptCore/jit/JITCall.cpp:564
> + using namespace BaselineGetByIdRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITCall.cpp:657
> + using namespace BaselineGetByIdRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITCall.cpp:674
> + notObject.append(branchIfNotObject(iterCallResultJSR));
We should pass iterCallResultJSR.payloadGPR() since we already know that this is a cell.
> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:132
> +static void generateGetByIdInlineAccess(JIT& jit, GPRReg stubInfoGPR, JSValueRegs baseJSR, GPRReg scratch, JSValueRegs resultJSR)
Let's name scratch scratchGPR.
> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:209
> + using namespace BaselinePutByIdRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:383
> + using namespace BaselineInByIdRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or, use like,
using BaselineInByIdRegisters::baseJSR;
> Source/JavaScriptCore/jit/JITInlines.h:369
> +inline void JIT::emitArrayProfilingSiteWithCell(const Bytecode& bytecode, ptrdiff_t offsetOfArrayProfile, JSValueRegs cellJSR, RegisterID scratchGPR)
We should take GPRReg instead of JSValueRegs.
> Source/JavaScriptCore/jit/JITInlines.h:378
> +inline void JIT::emitArrayProfilingSiteWithCell(const Bytecode& bytecode, JSValueRegs cellJSR, RegisterID scratchGPR)
We should take GPRReg instead of JSValueRegs.
> Source/JavaScriptCore/jit/JITOpcodes32_64.cpp:155
> + using namespace BaselineInstanceofRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:55
> + using namespace BaselineGetByValRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:62
> + emitArrayProfilingSiteWithCell(bytecode, baseJSR, scratchGPR);
We should pass baseJSR.payloadGPR() here since it only accepts Cell GPR.
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:68
> + emitArrayProfilingSiteWithCell(bytecode, baseJSR, scratchGPR);
We should pass baseJSR.payloadGPR() here since it only accepts Cell GPR.
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:113
> + constexpr GPRReg scratchGPR = preferredArgumentGPR<SlowOperation, 2>();
Let's name it profileGPR.
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:122
> + materializePointerIntoMetadata(bytecode, OpcodeType::Metadata::offsetOfArrayProfile(), scratchGPR);
Ditto. profileGPR.
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:209
> + using namespace BaselineGetByValRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:331
> + using namespace BaselinePrivateBrandRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:394
> + constexpr GPRReg propertyGPR = BaselinePrivateBrandRegisters::brandJSR.payloadGPR();
We should rename it to brandGPR.
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:396
> static_assert(propertyGPR == argumentGPR1);
Ditto.
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:413
> + using namespace BaselinePrivateBrandRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:491
> + using namespace BaselinePutByValRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:498
> + emitArrayProfilingSiteWithCell(bytecode, baseJSR, profileGPR);
We should pass baseJSR.payloadGPR().
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:573
> + emitGetVirtualRegister(base, baseJSR);
> + emitGetVirtualRegister(property, propertyJSR);
> + emitGetVirtualRegister(value, valueJSR);
Let's move these things after `materializePointerIntoMetadata` related thing to make it aligned to the other code for the slow path.
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:659
> + using namespace BaselinePutByValRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:713
> + loadConstant(gen.m_unlinkedStubInfoConstantIndex, stubInfoGPR);
Let's load constant registers first before emitGetVirtualRegisters (just after loadGlobalObject) to align it to the other places.
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:839
> + using namespace BaselineDelByIdRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:970
> + using namespace BaselineDelByValRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1104
> + using namespace BaselineGetByIdRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1186
> + using namespace BaselineGetByIdRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1233
> + Address(argumentGPR1, StructureStubInfo::offsetOfSlowOperation()),
Let's use stubInfoGPR instead of argumentGPR1.
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1270
> + using namespace BaselineGetByIdRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1278
> + emitArrayProfilingSiteWithCell(bytecode, OpGetById::Metadata::offsetOfModeMetadata() + GetByIdModeMetadataArrayLength::offsetOfArrayProfile(), baseJSR, scratchGPR);
We should pass baseJSR.payloadGPR().
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1362
> + using namespace BaselineGetByIdWithThisRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1455
> + Address(argumentGPR1, StructureStubInfo::offsetOfSlowOperation()),
Let's use stubInfoGPR instead of argumentGPR1.
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1541
> + using namespace BaselinePutByIdRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1669
> + using namespace BaselineInByIdRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1752
> + using namespace BaselineInByValRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1757
> + emitArrayProfilingSiteWithCell(bytecode, baseJSR, scratchGPR);
We should pass baseJSR.payloadGPR().
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1837
> + using namespace BaselineInByValRegisters;
We are not encouraging `using namespace`. Let's define baseJSR etc. as the same way to the above old code.
Or using (not using namespace).
> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:3174
> + emitArrayProfilingSiteWithCell(bytecode, JSValueRegs { baseGPR }, scratch1);
We should pass baseGPR.
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211022/a5bfcf05/attachment-0001.htm>
More information about the webkit-unassigned
mailing list