Bug 206577 - [JSC] Attempt to fix BytecodeIndex handling in 32bit
Summary: [JSC] Attempt to fix BytecodeIndex handling in 32bit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: DoNotImportToRadar
Depends on:
Blocks: 206602
  Show dependency tree / graph
 
Reported: 2020-01-22 03:25 PST by Yusuke Suzuki
Modified: 2020-01-22 15:59 PST (History)
13 users (show)

See Also:

Attachments
Patch (18.81 KB, patch)
2020-01-22 03:29 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (19.25 KB, patch)
2020-01-22 03:37 PST, Yusuke Suzuki 		no flags Details | Formatted Diff | Diff
Patch (19.37 KB, patch)
2020-01-22 03:48 PST, Yusuke Suzuki 		keith_miller: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-01-22 03:25:58 PST 
[JSC] Attempt to fix BytecodeIndex handling in 32bit
Comment 1 Yusuke Suzuki 2020-01-22 03:29:36 PST 
Created attachment 388411 [details]
Patch
Comment 2 Yusuke Suzuki 2020-01-22 03:37:20 PST 
Created attachment 388413 [details]
Patch
Comment 3 Yusuke Suzuki 2020-01-22 03:48:11 PST 
Created attachment 388414 [details]
Patch
Comment 4 Caio Lima 2020-01-22 04:45:50 PST 
Comment on attachment 388414 [details]
Patch

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

LGTM besides an issue I've found. Regarding 32-bits changes, EWS bots won't test them, but we have a WIP patch to re-enable JIT on ARMv7 and MIPS and we've applies those changes as well.

> Source/JavaScriptCore/llint/LLIntData.cpp:132
> +    ASSERT(CodeBlock::llintBaselineCalleeSaveSpaceAsVirtualRegisters() == 2);

The way we calculate `llintBaselineCalleeSaveSpaceAsVirtualRegisters` will make this fail. The problem is that on `roundCalleeSaveSpaceAsVirtualRegisters` we are using `WTF::roundUpToMultipleOf(sizeof(Register), calleeSaveRegisters * sizeof(CPURegister)) / sizeof(Register)`. Having 2 callee-save registers on 32-bits still returns 1.
Comment 5 Caio Lima 2020-01-22 06:20:48 PST 
BTW, I just noticed that we missed adding entries `preserveCalleeSavesUsedByLLInt()` and `restoreCalleeSavesUsedByLLInt()` for the new callee-save PB. Could you add those there?
Comment 6 Keith Miller 2020-01-22 10:02:58 PST 
Comment on attachment 388414 [details]
Patch

r=me.
Comment 7 Yusuke Suzuki 2020-01-22 12:28:20 PST 
Comment on attachment 388414 [details]
Patch

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

Fixed preserveCalleeSavesUsedByLLInt etc. I'm thinking this is also wrong in CLoop. We should fix it in a separate patch.

>> Source/JavaScriptCore/llint/LLIntData.cpp:132
>> +    ASSERT(CodeBlock::llintBaselineCalleeSaveSpaceAsVirtualRegisters() == 2);
> 
> The way we calculate `llintBaselineCalleeSaveSpaceAsVirtualRegisters` will make this fail. The problem is that on `roundCalleeSaveSpaceAsVirtualRegisters` we are using `WTF::roundUpToMultipleOf(sizeof(Register), calleeSaveRegisters * sizeof(CPURegister)) / sizeof(Register)`. Having 2 callee-save registers on 32-bits still returns 1.

Right. Fixed.
Comment 8 Yusuke Suzuki 2020-01-22 12:28:43 PST 
(In reply to Yusuke Suzuki from comment #7)
> Comment on attachment 388414 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388414&action=review
> 
> Fixed preserveCalleeSavesUsedByLLInt etc. I'm thinking this is also wrong in
> CLoop. We should fix it in a separate patch.

Maybe no. Need to check.
Comment 9 Yusuke Suzuki 2020-01-22 12:31:37 PST 
Committed r254934: <https://trac.webkit.org/changeset/254934>
Comment 10 Radar WebKit Bug Importer 2020-01-22 15:57:50 PST 
<rdar://problem/58814703>