Bug 203563 – [JSC] 32-bit platforms should use a PC base register

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Zan Dobersek]

This way the CallSiteIndex offset values could be used the same way as on 64-bit platforms.

More about this in bug #203358.

[Caio Lima]

Created attachment 387651 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 387651 [details]
Patch

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

Early comment.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:299
> +        const PB = csr1

You need to add this register to sets of RegisterSet.cpp.
Let's read the comment in RegisterSet.h.

 51     JS_EXPORT_PRIVATE static RegisterSet stackRegisters();
 52     JS_EXPORT_PRIVATE static RegisterSet reservedHardwareRegisters();
 53     static RegisterSet runtimeTagRegisters();
 54     static RegisterSet specialRegisters(); // The union of stack, reserved hardware, and runtime registers.
 55     JS_EXPORT_PRIVATE static RegisterSet calleeSaveRegisters();
 56     static RegisterSet vmCalleeSaveRegisters(); // Callee save registers that might be saved and used by any tier.
 57     static RegisterAtOffsetList* vmCalleeSaveRegisterOffsets();
 58     static RegisterSet llintBaselineCalleeSaveRegisters(); // Registers saved and used by the LLInt. 
 59     static RegisterSet dfgCalleeSaveRegisters(); // Registers saved and used by the DFG JIT.
 60     static RegisterSet ftlCalleeSaveRegisters(); // Registers that might be saved and used by the FTL JIT.
 61 #if ENABLE(WEBASSEMBLY)
 62     static RegisterSet webAssemblyCalleeSaveRegisters(); // Registers saved and used by the WebAssembly JIT.
 63 #endif
 64     static RegisterSet volatileRegistersForJSCall();
 65     static RegisterSet stubUnavailableRegisters(); // The union of callee saves and special registers.
 66     JS_EXPORT_PRIVATE static RegisterSet macroScratchRegisters();
 67     JS_EXPORT_PRIVATE static RegisterSet allGPRs();
 68     JS_EXPORT_PRIVATE static RegisterSet allFPRs();
 69     static RegisterSet allRegisters();
 70     JS_EXPORT_PRIVATE static RegisterSet argumentGPRS();
 71 
 72     static RegisterSet registersToNotSaveForJSCall();
 73     static RegisterSet registersToNotSaveForCCall();

We need to add this register to appropriate sets.

> Source/JavaScriptCore/offlineasm/arm.rb:73
>  ARM_EXTRA_FPRS = [SpecialRegister.new("d7")]

Let's update the comments in arm.rb about csr.

> Source/JavaScriptCore/offlineasm/arm.rb:105
> +            "r3"

Looks correct. Keep in sync with GPRInfo.h

> Source/JavaScriptCore/offlineasm/arm.rb:115
> +        when "csr1"
> +            "r10"

GPRInfo.h and RegisterSet.cpp need to be updated.

> Source/JavaScriptCore/offlineasm/mips.rb:138
>              "$fp"

Let's update the comment in mips.rb. And keep in sync with GPRInfo.h. And add registers to appropriate set in RegisterSet.cpp.

[Yusuke Suzuki]

When modifying ARMv7 register definition, keep in mind that ARM registers usage are slightly different in iOS ARM and Linux ARM. See ARMv7Registers.h.

[Yusuke Suzuki]

Comment on attachment 387651 [details]
Patch

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

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:912
> +            loadp 4[temp], csr1

This means you need to update GPRInfo.h's NUMBER_OF_CALLEE_SAVES_REGISTERS too.

[Caio Lima]

Created attachment 387777 [details]
Patch

[Caio Lima]

Created attachment 387779 [details]
WIP - Patch

[Caio Lima]

@Yusuke, Thank you very much for the comments.

[Caio Lima]

Created attachment 387811 [details]
Patch

[Caio Lima]

Comment on attachment 387651 [details]
Patch

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

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:299
>> +        const PB = csr1
> 
> You need to add this register to sets of RegisterSet.cpp.
> Let's read the comment in RegisterSet.h.
> 
>  51     JS_EXPORT_PRIVATE static RegisterSet stackRegisters();
>  52     JS_EXPORT_PRIVATE static RegisterSet reservedHardwareRegisters();
>  53     static RegisterSet runtimeTagRegisters();
>  54     static RegisterSet specialRegisters(); // The union of stack, reserved hardware, and runtime registers.
>  55     JS_EXPORT_PRIVATE static RegisterSet calleeSaveRegisters();
>  56     static RegisterSet vmCalleeSaveRegisters(); // Callee save registers that might be saved and used by any tier.
>  57     static RegisterAtOffsetList* vmCalleeSaveRegisterOffsets();
>  58     static RegisterSet llintBaselineCalleeSaveRegisters(); // Registers saved and used by the LLInt. 
>  59     static RegisterSet dfgCalleeSaveRegisters(); // Registers saved and used by the DFG JIT.
>  60     static RegisterSet ftlCalleeSaveRegisters(); // Registers that might be saved and used by the FTL JIT.
>  61 #if ENABLE(WEBASSEMBLY)
>  62     static RegisterSet webAssemblyCalleeSaveRegisters(); // Registers saved and used by the WebAssembly JIT.
>  63 #endif
>  64     static RegisterSet volatileRegistersForJSCall();
>  65     static RegisterSet stubUnavailableRegisters(); // The union of callee saves and special registers.
>  66     JS_EXPORT_PRIVATE static RegisterSet macroScratchRegisters();
>  67     JS_EXPORT_PRIVATE static RegisterSet allGPRs();
>  68     JS_EXPORT_PRIVATE static RegisterSet allFPRs();
>  69     static RegisterSet allRegisters();
>  70     JS_EXPORT_PRIVATE static RegisterSet argumentGPRS();
>  71 
>  72     static RegisterSet registersToNotSaveForJSCall();
>  73     static RegisterSet registersToNotSaveForCCall();
> 
> We need to add this register to appropriate sets.

Done.

>> Source/JavaScriptCore/offlineasm/arm.rb:73
>>  ARM_EXTRA_FPRS = [SpecialRegister.new("d7")]
> 
> Let's update the comments in arm.rb about csr.

Done.

>> Source/JavaScriptCore/offlineasm/arm.rb:105
>> +            "r3"
> 
> Looks correct. Keep in sync with GPRInfo.h

Using `r4` here was divergent from `GPRInfo.h`. this change makes them aligned again.

>> Source/JavaScriptCore/offlineasm/arm.rb:115
>> +            "r10"
> 
> GPRInfo.h and RegisterSet.cpp need to be updated.

Done.

>> Source/JavaScriptCore/offlineasm/mips.rb:138
>>              "$fp"
> 
> Let's update the comment in mips.rb. And keep in sync with GPRInfo.h. And add registers to appropriate set in RegisterSet.cpp.

Done.

[Caio Lima]

Created attachment 387828 [details]
Patch

[Caio Lima]

Created attachment 387844 [details]
Patch

[Keith Miller]

Comment on attachment 387844 [details]
Patch

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

r=me with some comments.

> Source/JavaScriptCore/assembler/MIPSRegisters.h:53
> +    macro(r17, "s1",   0, 1)                    \

Was this just a bug in the MIPS calling convention?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:66
>  macro nextInstruction()
> -    loadb [PC], t0
> +    loadb [PB, PC, 1], t0
>      leap _g_opcodeMap, t1
>      jmp [t1, t0, 4], BytecodePtrTag
>  end
>  
>  macro nextInstructionWide16()
> -    loadb OpcodeIDNarrowSize[PC], t0
> +    loadb OpcodeIDNarrowSize[PB, PC, 1], t0
>      leap _g_opcodeMapWide16, t1
>      jmp [t1, t0, 4], BytecodePtrTag
>  end
>  
>  macro nextInstructionWide32()
> -    loadb OpcodeIDNarrowSize[PC], t0
> +    loadb OpcodeIDNarrowSize[PB, PC, 1], t0
>      leap _g_opcodeMapWide32, t1
>      jmp [t1, t0, 4], BytecodePtrTag
>  end
>  
>  macro getuOperandNarrow(opcodeStruct, fieldName, dst)
> -    loadb constexpr %opcodeStruct%_%fieldName%_index + OpcodeIDNarrowSize[PC], dst
> +    loadb constexpr %opcodeStruct%_%fieldName%_index + OpcodeIDNarrowSize[PB, PC, 1], dst
>  end
>  
>  macro getOperandNarrow(opcodeStruct, fieldName, dst)
> -    loadbsi constexpr %opcodeStruct%_%fieldName%_index + OpcodeIDNarrowSize[PC], dst
> +    loadbsi constexpr %opcodeStruct%_%fieldName%_index + OpcodeIDNarrowSize[PB, PC, 1], dst
>  end
>  
>  macro getuOperandWide16(opcodeStruct, fieldName, dst)
> -    loadh constexpr %opcodeStruct%_%fieldName%_index * 2 + OpcodeIDWide16Size[PC], dst
> +    loadh constexpr %opcodeStruct%_%fieldName%_index * 2 + OpcodeIDWide16Size[PB, PC, 1], dst
>  end
>  
>  macro getOperandWide16(opcodeStruct, fieldName, dst)
> -    loadhsi constexpr %opcodeStruct%_%fieldName%_index * 2 + OpcodeIDWide16Size[PC], dst
> +    loadhsi constexpr %opcodeStruct%_%fieldName%_index * 2 + OpcodeIDWide16Size[PB, PC, 1], dst
>  end
>  
>  macro getuOperandWide32(opcodeStruct, fieldName, dst)
> -    loadi constexpr %opcodeStruct%_%fieldName%_index * 4 + OpcodeIDWide32Size[PC], dst
> +    loadi constexpr %opcodeStruct%_%fieldName%_index * 4 + OpcodeIDWide32Size[PB, PC, 1], dst
>  end
>  
>  macro getOperandWide32(opcodeStruct, fieldName, dst)
> -    loadis constexpr %opcodeStruct%_%fieldName%_index * 4 + OpcodeIDWide32Size[PC], dst
> +    loadis constexpr %opcodeStruct%_%fieldName%_index * 4 + OpcodeIDWide32Size[PB, PC, 1], dst
>  end

Can you add a fix me to merge this with the ones in LowLevelInterpreter64.asm? Seems like you can't right now because we don't have a loadhsp (although, I think loadhsi is probably fine).

[Keith Miller]

This also fixes the crash I was seeing on our watch builds. So I think we should land this while undoing my revert.

[Caio Lima]

Created attachment 387905 [details]
Patch

[Caio Lima]

Comment on attachment 387844 [details]
Patch

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

Thank you very much for the review.

>> Source/JavaScriptCore/assembler/MIPSRegisters.h:53
>> +    macro(r17, "s1",   0, 1)                    \
> 
> Was this just a bug in the MIPS calling convention?

I think it would be a problem if we were using such register at some point, but it seems we don't use `$sx` registers besides `s0` (and now `s1`).

>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:66
>>  end
> 
> Can you add a fix me to merge this with the ones in LowLevelInterpreter64.asm? Seems like you can't right now because we don't have a loadhsp (although, I think loadhsi is probably fine).

Done.

[Caio Lima]

Comment on attachment 387844 [details]
Patch

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

>>> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:66
>>>  end
>> 
>> Can you add a fix me to merge this with the ones in LowLevelInterpreter64.asm? Seems like you can't right now because we don't have a loadhsp (although, I think loadhsi is probably fine).
> 
> Done.

I've refactored `nextInstruction` part out to LowLevelInterpreter.asm, since 64 and 32-bits are the same.

[WebKit Commit Bot]

Comment on attachment 387905 [details]
Patch

Clearing flags on attachment: 387905

Committed r254674: <https://trac.webkit.org/changeset/254674>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58641478>