[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