Bug 190273 - Support arm64 CPUs with a 32-bit address space in the LLInt
Summary: Support arm64 CPUs with a 32-bit address space in the LLInt
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: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-03 20:19 PDT by Keith Miller
Modified: 2018-10-16 08:23 PDT (History)
10 users (show)

See Also:


Attachments
WIP (73.69 KB, patch)
2018-10-03 20:37 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (120.61 KB, patch)
2018-10-15 19:44 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (121.60 KB, patch)
2018-10-15 20:26 PDT, Keith Miller
msaboff: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2018-10-03 20:19:17 PDT
Support arm64 CPUs with a 32-bit address space
Comment 1 Keith Miller 2018-10-03 20:37:16 PDT
Created attachment 351577 [details]
WIP
Comment 2 Keith Miller 2018-10-15 19:44:02 PDT
Created attachment 352426 [details]
Patch
Comment 3 Keith Miller 2018-10-15 20:26:51 PDT
Created attachment 352428 [details]
Patch
Comment 4 Keith Miller 2018-10-15 20:29:05 PDT
Comment on attachment 352428 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3


I'll change this title before landing.
Comment 5 Michael Saboff 2018-10-15 21:05:44 PDT
Comment on attachment 352428 [details]
Patch

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

r=me with a few questions.

> Source/JavaScriptCore/interpreter/CallFrame.h:73
> +        alignas(CPURegister) CallFrame* callerFrame;
> +        alignas(CPURegister) Instruction* returnPC;

Make this camel case: alignAs()

> Source/JavaScriptCore/offlineasm/arm64.rb:83
> +    when :word

Hmm.  Not sure I like full register kind to be "word".  How about "reg", "full" or "fullReg"?
Comment 6 Keith Miller 2018-10-15 22:11:24 PDT
Comment on attachment 352428 [details]
Patch

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

>> Source/JavaScriptCore/offlineasm/arm64.rb:83
>> +    when :word
> 
> Hmm.  Not sure I like full register kind to be "word".  How about "reg", "full" or "fullReg"?

How about changing it to “quad” and “int” to “word” as that’s how the abi refers to them?
Comment 7 Saam Barati 2018-10-15 22:37:41 PDT
Comment on attachment 352428 [details]
Patch

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

LGTM too

> Source/JavaScriptCore/assembler/CPU.h:34
> +using CPURegister = int64_t;
> +using UCPURegister = uint64_t;

Do we really need both of these?

> Source/JavaScriptCore/b3/testb3.cpp:14611
> +#if CPU(ADDRESS64)

Why? This test seems like it should work.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4413
> +            m_jit.mul32(TrustedImm32(sizeof(HasOwnPropertyCache::Entry)), hashGPR, hashGPR);

Why not just pad the struct?

> Source/JavaScriptCore/interpreter/CalleeBits.h:54
> +        CalleeBits result(reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(callee) | TagBitsWasm));

Not sure this is the best way to do this. If we're ADDRESS32, we can box into 64 bits a lot easier.

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:918
> +    if ARM64 and not ADDRESS64
> +        subi MachineRegisterSize, temp2
> +        loadq [sp, temp2, 1], temp3
> +        storeq temp3, [temp1, temp2, 1]
> +        btinz temp2, .copyLoop
> +    else
> +        subi PtrSize, temp2
> +        loadp [sp, temp2, 1], temp3
> +        storep temp3, [temp1, temp2, 1]
> +        btinz temp2, .copyLoop
> +    end

Is this actually needed? Doesn't ARM64_32 have 64 bit stores?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:552
> +    # Adds to sp are always 64 bit on arm64 so we need t0's high bits to stay marked.

what do you mean marked?

> Source/JavaScriptCore/runtime/SlowPathReturnType.h:47
> +    result.a = reinterpret_cast<CPURegister>(a);
> +    result.b = reinterpret_cast<CPURegister>(b);

Does the ABI guarantee anything about the upper bits here? Should we zero extend to be safe?
Comment 8 Keith Miller 2018-10-16 00:07:43 PDT
Comment on attachment 352428 [details]
Patch

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

>> Source/JavaScriptCore/assembler/CPU.h:34
>> +using UCPURegister = uint64_t;
> 
> Do we really need both of these?

If you want an unsigned value, which some cases seem to want for shifting purposes.

>> Source/JavaScriptCore/b3/testb3.cpp:14611
>> +#if CPU(ADDRESS64)
> 
> Why? This test seems like it should work.

What does it mean to shift a pointer by 32 when that's the size of your address space?

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4413
>> +            m_jit.mul32(TrustedImm32(sizeof(HasOwnPropertyCache::Entry)), hashGPR, hashGPR);
> 
> Why not just pad the struct?

Why waste the memory? It's not that much slower to do a multiply.

>> Source/JavaScriptCore/interpreter/CallFrame.h:73
>> +        alignas(CPURegister) Instruction* returnPC;
> 
> Make this camel case: alignAs()

It's a builtin C++ macro value.

>> Source/JavaScriptCore/interpreter/CalleeBits.h:54
>> +        CalleeBits result(reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(callee) | TagBitsWasm));
> 
> Not sure this is the best way to do this. If we're ADDRESS32, we can box into 64 bits a lot easier.

The tag bits are on the bottom anyway.

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:918
>> +    end
> 
> Is this actually needed? Doesn't ARM64_32 have 64 bit stores?

Yes that's why I do the q loads and stores for ARM64 when not Address64.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:552
>> +    # Adds to sp are always 64 bit on arm64 so we need t0's high bits to stay marked.
> 
> what do you mean marked?

if t0 contains a negative value we should maintain the 1s in the upper 32 bits. I guess I could change this to: 

# Adds to sp are always 64 bit on arm64 so we need avoid zeroing t0's high bits.

>> Source/JavaScriptCore/runtime/SlowPathReturnType.h:47
>> +    result.b = reinterpret_cast<CPURegister>(b);
> 
> Does the ABI guarantee anything about the upper bits here? Should we zero extend to be safe?

The ABI is caller zero's the upper 32-bits.
Comment 9 Keith Miller 2018-10-16 00:19:23 PDT
Committed r237173: <https://trac.webkit.org/changeset/237173>
Comment 10 Radar WebKit Bug Importer 2018-10-16 00:20:26 PDT
<rdar://problem/45298718>
Comment 11 Alex Christensen 2018-10-16 07:04:23 PDT
It looks like this broke the i386 build even though it worked on EWS :(
Comment 12 Keith Miller 2018-10-16 08:23:29 PDT
(In reply to Alex Christensen from comment #11)
> It looks like this broke the i386 build even though it worked on EWS :(

Hmm, weird. Could be it needs a clean build.