Bug 232079 – [JSC][32bit] Use DataIC in Baseline JIT

• Component: New Bugs
• Status: RESOLVED FIXED

[Geza Lore]

[JSC][32bit] Use DataIC in Baseline JIT

[Geza Lore]

Created attachment 442029 [details]
Patch

[Geza Lore]

Created attachment 442042 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 442042 [details]
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.

[Geza Lore]

Created attachment 442137 [details]
Patch

[Geza Lore]

Comment on attachment 442042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442042&action=review

>> Source/JavaScriptCore/jit/JITCall.cpp:488
>>  
> 
> 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.

returnValueJSR is generic and defined at the top of the file as the JSValueRegs holding the ABI return value register/registers. We want to pass the result of the function call above to the IC generator, which happen to be in returnValueGPR (ABI), which also happens to be the same as regT0 (GPRInfo.h), and baseGPR also was defined to regT0 (JITInlineCacheGenerator.h), so that move emitted nothing and it was just a roundabout way of saying pass in the return value of the previous function call that I can't see there is a need for.

With that in mind, I left this in.

[Geza Lore]

All other requested changes are done.

[Yusuke Suzuki]

Comment on attachment 442137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442137&action=review

r=me with comment.

> Source/JavaScriptCore/jit/JITCall.cpp:-415
> -    emitJumpSlowCaseIfNotJSCell(baseGPR);

We should insert `static_assert(BaselineGetByIdRegisters::baseJSR == BaselineGetByIdRegisters::resultJSR);`.
We have baseJSR. And it is possible that we will change baseJSR while keeping resultJSR value.
Then, it is wrong that this code's base value register is not changed. To notice that we are using returnValueJSR here instead, we should put static_assert so that compilation error will happen.
baseJSR and resultJSR are the same, but this is not guaranteed thing.

[Geza Lore]

Created attachment 442355 [details]
Patch

[EWS]

Committed r284781 (243490@main): <https://commits.webkit.org/243490@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442355 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84612428>

[Saam Barati]

Comment on attachment 442355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442355&action=review

Nice, LGTM too. This patch is great

> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:309
> +        // FIXME: This scratch register is not generally safe to use on ARMv7, as the macro
> +        //        assembler always assumes it is available. At the moment, it does happen to work
> +        //        with the code below.

this is the same for all the macro assemblers, hence the DisallowMacroScratchRegisterUsage scope. Don't think we need a FIXME here

> Source/JavaScriptCore/jit/CCallHelpers.h:874
> +    template <typename OperationType, unsigned ArgNum, unsigned Index = ArgNum, typename... Args>
> +    static constexpr JSValueRegs pickJSR(GPRReg first, Args... rest)
> +    {
> +        static_assert(sizeOfArg<OperationType, ArgNum - Index>() <= 8, "Don't know how to handle large arguments");
> +        if constexpr (!Index)
> +            return JSValueRegs { first };
> +        else {
> +            UNUSED_PARAM(first); // Otherwise warning due to constexpr
> +            return pickJSR<OperationType, ArgNum, Index - 1>(rest...);
> +        }
> +    }

can't this just construct a tuple and return std::get<Index>?

[Geza Lore]

Comment on attachment 442355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442355&action=review

>> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:309
>> +        //        with the code below.
> 
> this is the same for all the macro assemblers, hence the DisallowMacroScratchRegisterUsage scope. Don't think we need a FIXME here

My comment is probably obtuse, but the reason this is unsafe is precisely because the ARMv7 macro assembler ignores DisallowMacroScratchRegisterUsage. If you look in MacroAssemblerARMv7.h, it unconditionally uses the 'dataTempRegister' all over without ever checking the m_allowScratchRegister flag, hence the FIXME. Same applies to MIPS and RISCV64. ARM64/ARM64E and x86 are OK.

>> Source/JavaScriptCore/jit/CCallHelpers.h:874
>> +    }
> 
> can't this just construct a tuple and return std::get<Index>?

I think this one (the JSVALUE64 version only) could, I admit I have not thought of that as I was thinking of the more generic algorithm that works for JSVALUE32_64 as well where you need to cherry pick your register in a more peculiar way when passing 64-bit values. The JSVALUE64 version is just a simplified version of that which indeed appears to be identical to the std::tuple method.

Maybe it is better to keep the two platforms to use the same algorithm structure so it's easier on the next reader of the JSVALUE32_64 code that is perhaps less obvious?

[Saam Barati]

Comment on attachment 442355 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=442355&action=review

>>> Source/JavaScriptCore/bytecode/CallLinkInfo.cpp:309
>>> +        //        with the code below.
>> 
>> this is the same for all the macro assemblers, hence the DisallowMacroScratchRegisterUsage scope. Don't think we need a FIXME here
> 
> My comment is probably obtuse, but the reason this is unsafe is precisely because the ARMv7 macro assembler ignores DisallowMacroScratchRegisterUsage. If you look in MacroAssemblerARMv7.h, it unconditionally uses the 'dataTempRegister' all over without ever checking the m_allowScratchRegister flag, hence the FIXME. Same applies to MIPS and RISCV64. ARM64/ARM64E and x86 are OK.

Yeah, we should make DisallowMacroScratchRegisterUsage a thing for those assemblers.

>>> Source/JavaScriptCore/jit/CCallHelpers.h:874
>>> +    }
>> 
>> can't this just construct a tuple and return std::get<Index>?
> 
> I think this one (the JSVALUE64 version only) could, I admit I have not thought of that as I was thinking of the more generic algorithm that works for JSVALUE32_64 as well where you need to cherry pick your register in a more peculiar way when passing 64-bit values. The JSVALUE64 version is just a simplified version of that which indeed appears to be identical to the std::tuple method.
> 
> Maybe it is better to keep the two platforms to use the same algorithm structure so it's easier on the next reader of the JSVALUE32_64 code that is perhaps less obvious?

I don't have a strong preference. I think it's fine to leave it as is.