Bug 203563

Summary: [JSC] 32-bit platforms should use a PC base register
Product: WebKit Reporter: Zan Dobersek <zan>
Component: JavaScriptCoreAssignee: Caio Lima <ticaiolima>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, guijemont, Hironori.Fujii, keith_miller, mark.lam, msaboff, saam, ticaiolima, tzagallo, webkit-bug-importer, xan.lopez, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 205581, 206010, 206231, 206602    
Attachments:
Description Flags
Patch
none
Patch
none
WIP - Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Zan Dobersek 2019-10-29 08:05:00 PDT 
This way the CallSiteIndex offset values could be used the same way as on 64-bit platforms.

More about this in bug #203358.
Comment 1 Caio Lima 2020-01-14 06:56:08 PST 
Created attachment 387651 [details]
Patch
Comment 2 Yusuke Suzuki 2020-01-15 00:50:37 PST 
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.
Comment 3 Yusuke Suzuki 2020-01-15 00:52:07 PST 
When modifying ARMv7 register definition, keep in mind that ARM registers usage are slightly different in iOS ARM and Linux ARM. See ARMv7Registers.h.
Comment 4 Yusuke Suzuki 2020-01-15 01:08:44 PST 
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.
Comment 5 Caio Lima 2020-01-15 04:54:46 PST 
Created attachment 387777 [details]
Patch
Comment 6 Caio Lima 2020-01-15 05:20:36 PST 
Created attachment 387779 [details]
WIP - Patch
Comment 7 Caio Lima 2020-01-15 05:41:47 PST 
@Yusuke, Thank you very much for the comments.
Comment 8 Caio Lima 2020-01-15 11:34:17 PST 
Created attachment 387811 [details]
Patch
Comment 9 Caio Lima 2020-01-15 12:52:21 PST 
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.
Comment 10 Caio Lima 2020-01-15 12:58:00 PST 
Created attachment 387828 [details]
Patch
Comment 11 Caio Lima 2020-01-15 14:13:14 PST 
Created attachment 387844 [details]
Patch
Comment 12 Keith Miller 2020-01-15 18:13:56 PST 
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).
Comment 13 Keith Miller 2020-01-15 18:14:52 PST 
This also fixes the crash I was seeing on our watch builds. So I think we should land this while undoing my revert.
Comment 14 Caio Lima 2020-01-16 03:31:55 PST 
Created attachment 387905 [details]
Patch
Comment 15 Caio Lima 2020-01-16 03:50:43 PST 
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.
Comment 16 Caio Lima 2020-01-16 04:34:35 PST 
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.
Comment 17 WebKit Commit Bot 2020-01-16 05:21:12 PST 
Comment on attachment 387905 [details]
Patch

Clearing flags on attachment: 387905

Committed r254674: <https://trac.webkit.org/changeset/254674>
Comment 18 WebKit Commit Bot 2020-01-16 05:21:14 PST 
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2020-01-16 05:22:14 PST 
<rdar://problem/58641478>