Bug 190273 – Support arm64 CPUs with a 32-bit address space in the LLInt

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

Support arm64 CPUs with a 32-bit address space

[Keith Miller]

Created attachment 351577 [details]
WIP

[Keith Miller]

Created attachment 352426 [details]
Patch

[Keith Miller]

Created attachment 352428 [details]
Patch

[Keith Miller]

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.

[Michael Saboff]

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"?

[Keith Miller]

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?

[Saam Barati]

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?

[Keith Miller]

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.

[Keith Miller]

Committed r237173: <https://trac.webkit.org/changeset/237173>

[Radar WebKit Bug Importer]

<rdar://problem/45298718>

[Alex Christensen]

It looks like this broke the i386 build even though it worked on EWS :(

[Keith Miller]

(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.