Bug 206611 – [ARMv7][JIT] Implement checkpoint support

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caio Lima]

...

[Caio Lima]

Created attachment 388469 [details]
WIP - Patch

This patch depends on patch uploaded on https://bugs.webkit.org/show_bug.cgi?id=206603 to fix build of JIT on 32-bit ports.

[Caio Lima]

Created attachment 388485 [details]
Patch

[Caio Lima]

Created attachment 388535 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 388535 [details]
Patch

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

Added comment.

> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310
> +#if USE(JSVALUE64)

Let's avoid adding `USE(JSVALUE64)` / `USE(JSVALUE32_64)` as much as possible when adding a new code. Use headerSizeInRegisters's number instead.
And we are also using `pivot` in the code above. Why is it OK?

[Caio Lima]

Comment on attachment 388535 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310
>> +#if USE(JSVALUE64)
> 
> Let's avoid adding `USE(JSVALUE64)` / `USE(JSVALUE32_64)` as much as possible when adding a new code. Use headerSizeInRegisters's number instead.
> And we are also using `pivot` in the code above. Why is it OK?

I'll update as requested, but I think current math for 32-bits is not correct yet. The usage of `pivot` before is fine because it will copy the entire CallFrame after the first 16-bytes of scratch. The problem is that on 32-bits, due to sizeof(intptr_t) == 4, the operation `bitwise_cast<intptr_t*>(pivot - 1) +  currentEntry.offsetAsIndex())` will clobber the last 4-bytes of pivot[-1](that points to ReturnPC) when offsetAsIndex is 1, while it will properly place value on pivot[0] on 64-bits. What we need to do is to place callee-save registers starting at pivot[0]. I'm illustrating below how we would like to have pivot populated on each architecture.

64-bits

pivot[-5] = ArgumentCountIncludingThis
pivot[-4] = Callee
pivot[-3] = CodeBlock
pivot[-2] = ReturnPC
pivot[-1] = callerFrame
pivot[0] = csr0
...

On 32-bits it needs to be

pivot[-4] = ArgumentCountIncludingThis
pivot[-3] = Callee
pivot[-2] = CodeBlock
pivot[-1] = CallerFrameAndReturnPC
pivot[0] = csr0/csr1
...

Current code is leaving a hole of 4-bytes on `pivot[0]`.

[Caio Lima]

Created attachment 388590 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 388590 [details]
Patch

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

Looks good, but put r- since I found some nits.

> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310
> +        if (sizeof(intptr_t) == 8)

This is not correct. In ARM64_32, we are using two 8 bytes slots for callFrame and PC while `sizeof(intptr_t) == 4`. See comment in CallFrame.h.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:615
> +                    case InPair:

This only exists for 32_64. So, let's write

#if USE(JSVALUE32_64)
    case InPair:
#endif

instead of using `#else` of `#if USE(JSVALUE64)`.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:634
> +#endif
> +                    case BooleanDisplacedInJSStack:
> +                    case CellDisplacedInJSStack:
>                      case DisplacedInJSStack: {
>                          sideState->tmps[i] = reinterpret_cast<JSValue*>(tmpScratch)[i + tmpOffset];
>                          break;
>                      }
> +#if USE(JSVALUE32_64)
> +                    case UnboxedCellInGPR: {
> +                        EncodedValueDescriptor* valueDescriptor = bitwise_cast<EncodedValueDescriptor*>(tmpScratch + i + tmpOffset);
> +                        sideState->tmps[i] = JSValue(JSValue::CellTag, valueDescriptor->asBits.payload);
> +                        break;
> +                    }
> +
> +                    case UnboxedBooleanInGPR: {
> +                        sideState->tmps[i] = jsBoolean(static_cast<bool>(tmpScratch[i + tmpOffset]));
> +                        break;
> +                    }
> +#endif

Having the same entries switched by `ifdef` is hard to read. Let's write it in a simpler way, like this.

case UnboxedCellInGPR: {
#if USE(JSVALUE64)
    sideState->tmps[i] = reinterpret_cast<JSValue*>(tmpScratch)[i + tmpOffset];
#else
    EncodedValueDescriptor* valueDescriptor = bitwise_cast<EncodedValueDescriptor*>(tmpScratch + i + tmpOffset);
    sideState->tmps[i] = JSValue(JSValue::CellTag, valueDescriptor->asBits.payload);
#endif
    break;
}

[Caio Lima]

Created attachment 388671 [details]
Patch

[Caio Lima]

Comment on attachment 388590 [details]
Patch

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

Thank you very much for the review.

>> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310
>> +        if (sizeof(intptr_t) == 8)
> 
> This is not correct. In ARM64_32, we are using two 8 bytes slots for callFrame and PC while `sizeof(intptr_t) == 4`. See comment in CallFrame.h.

I see. I was not aware of this architecture until now. Do we use JIT on ARM64_32? I'm changing this condition to check CallFrame::headerSizeInSlots, but none of those operations will work properly on ARM64_32.

>> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:615
>> +                    case InPair:
> 
> This only exists for 32_64. So, let's write
> 
> #if USE(JSVALUE32_64)
>     case InPair:
> #endif
> 
> instead of using `#else` of `#if USE(JSVALUE64)`.

Done.

>> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:634
>> +#endif
> 
> Having the same entries switched by `ifdef` is hard to read. Let's write it in a simpler way, like this.
> 
> case UnboxedCellInGPR: {
> #if USE(JSVALUE64)
>     sideState->tmps[i] = reinterpret_cast<JSValue*>(tmpScratch)[i + tmpOffset];
> #else
>     EncodedValueDescriptor* valueDescriptor = bitwise_cast<EncodedValueDescriptor*>(tmpScratch + i + tmpOffset);
>     sideState->tmps[i] = JSValue(JSValue::CellTag, valueDescriptor->asBits.payload);
> #endif
>     break;
> }

Done.

[Yusuke Suzuki]

Comment on attachment 388590 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310
>>> +        if (sizeof(intptr_t) == 8)
>> 
>> This is not correct. In ARM64_32, we are using two 8 bytes slots for callFrame and PC while `sizeof(intptr_t) == 4`. See comment in CallFrame.h.
> 
> I see. I was not aware of this architecture until now. Do we use JIT on ARM64_32? I'm changing this condition to check CallFrame::headerSizeInSlots, but none of those operations will work properly on ARM64_32.

We are not using JIT for ARM64_32 while we are using LLInt. But we are aware of this architecture and I don't think using `sizeof(intptr_t)` here is a good idea.

[Yusuke Suzuki]

Comment on attachment 388671 [details]
Patch

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

r=me with nits.

> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310
> +        if (CallFrame::headerSizeInRegisters == 5)

Let's use

if constexpr (CallFrame::CallerFrameAndPC::sizeInRegisters == 2) {
} else {
}

Since CallerFrameAndPC is the problematic part.

> Source/JavaScriptCore/jit/GPRInfo.h:536
> +    static constexpr unsigned numberOfRegisters = 9;

Nice.

[Caio Lima]

(In reply to Yusuke Suzuki from comment #10)
> Comment on attachment 388590 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388590&action=review
> 
> >>> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310
> >>> +        if (sizeof(intptr_t) == 8)
> >> 
> >> This is not correct. In ARM64_32, we are using two 8 bytes slots for callFrame and PC while `sizeof(intptr_t) == 4`. See comment in CallFrame.h.
> > 
> > I see. I was not aware of this architecture until now. Do we use JIT on ARM64_32? I'm changing this condition to check CallFrame::headerSizeInSlots, but none of those operations will work properly on ARM64_32.
> 
> We are not using JIT for ARM64_32 while we are using LLInt. But we are aware
> of this architecture and I don't think using `sizeof(intptr_t)` here is a
> good idea.

Thanks for the heads up and I agree. I updated the patch to use `headerSizeInRegisters` instead and placed some assertions there as well. BTW< I saw the comment in CallFrame.h before, but my understanding was that caller frame and return pc together uses 8 bytes (like it is on ARMv7 and MIPS). Maybe we should change it to `arm64_32 expects caller frame and return pc to use 8 bytes each`.

[Caio Lima]

Created attachment 388706 [details]
Patch

[Caio Lima]

Comment on attachment 388671 [details]
Patch

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

Thanks again for the review.

>> Source/JavaScriptCore/dfg/DFGOSREntry.cpp:310
>> +        if (CallFrame::headerSizeInRegisters == 5)
> 
> Let's use
> 
> if constexpr (CallFrame::CallerFrameAndPC::sizeInRegisters == 2) {
> } else {
> }
> 
> Since CallerFrameAndPC is the problematic part.

Done.

[WebKit Commit Bot]

Comment on attachment 388706 [details]
Patch

Clearing flags on attachment: 388706

Committed r255088: <https://trac.webkit.org/changeset/255088>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58877206>