Bug 210870 - [JSC] branchIfBigInt32 can use BigInt32Mask and remove branchIfNumber filter
Summary: [JSC] branchIfBigInt32 can use BigInt32Mask and remove branchIfNumber filter
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: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-22 12:12 PDT by Yusuke Suzuki
Modified: 2020-04-22 20:12 PDT (History)
8 users (show)

See Also:


Attachments
Patch (17.58 KB, patch)
2020-04-22 12:15 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (17.58 KB, patch)
2020-04-22 12:38 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (19.67 KB, patch)
2020-04-22 13:13 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (20.93 KB, patch)
2020-04-22 13:56 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (20.93 KB, patch)
2020-04-22 14:09 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (21.11 KB, patch)
2020-04-22 15:07 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2020-04-22 12:12:03 PDT
[JSC] branchIfBigInt32 can use BigInt32Mask and remove branchIfNumber filter
Comment 1 Yusuke Suzuki 2020-04-22 12:15:01 PDT
Created attachment 397232 [details]
Patch
Comment 2 Robin Morisset 2020-04-22 12:36:20 PDT
Comment on attachment 397232 [details]
Patch

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

> Source/JavaScriptCore/jit/AssemblyHelpers.h:938
> +        and64(TrustedImm32(JSValue::BigInt32Mask), tempGPR);

It cannot be TrustedImm32, since it is a 64 bit mask. It has to be materialized one way or another.
Comment 3 Yusuke Suzuki 2020-04-22 12:38:33 PDT
Created attachment 397238 [details]
Patch
Comment 4 Yusuke Suzuki 2020-04-22 12:40:06 PDT
Comment on attachment 397232 [details]
Patch

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

>> Source/JavaScriptCore/jit/AssemblyHelpers.h:938
>> +        and64(TrustedImm32(JSValue::BigInt32Mask), tempGPR);
> 
> It cannot be TrustedImm32, since it is a 64 bit mask. It has to be materialized one way or another.

Fixed.
Comment 5 Yusuke Suzuki 2020-04-22 13:13:52 PDT
Created attachment 397244 [details]
Patch
Comment 6 Yusuke Suzuki 2020-04-22 13:23:07 PDT
Comment on attachment 397244 [details]
Patch

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

> Source/JavaScriptCore/jit/AssemblyHelpers.h:934
> +    Jump branchIfBigInt32(GPRReg gpr, GPRReg tempGPR)

I would extend it further by using number-tag-register later.

if (mode == HaveTagRegisters) {
move(GPRInfo::numberTagRegister, tempGPR);
add64(TrustedImm32(JSValue::BigInt32Tag), tempGPR);
and64(gpr, tempGPR);
return branch64(Equal, tempGPR, TrustedImm32(JSValue::BigInt32Tag));
}
Comment 7 Yusuke Suzuki 2020-04-22 13:26:38 PDT
Comment on attachment 397244 [details]
Patch

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

>> Source/JavaScriptCore/jit/AssemblyHelpers.h:934
>> +    Jump branchIfBigInt32(GPRReg gpr, GPRReg tempGPR)
> 
> I would extend it further by using number-tag-register later.
> 
> if (mode == HaveTagRegisters) {
> move(GPRInfo::numberTagRegister, tempGPR);
> add64(TrustedImm32(JSValue::BigInt32Tag), tempGPR);
> and64(gpr, tempGPR);
> return branch64(Equal, tempGPR, TrustedImm32(JSValue::BigInt32Tag));
> }

Or further,

if (mode == HaveTagRegisters) {
    add64(TrustedImm32(JSValue::BigInt32Tag), GPRInfo::numberTagRegister, tempGPR);
    and64(gpr, tempGPR);
    return branch64(Equal, tempGPR, TrustedImm32(JSValue::BigInt32Tag));
}
Comment 8 Yusuke Suzuki 2020-04-22 13:27:35 PDT
Comment on attachment 397244 [details]
Patch

I'll do it.
Comment 9 Saam Barati 2020-04-22 13:31:29 PDT
Comment on attachment 397244 [details]
Patch

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

>>> Source/JavaScriptCore/jit/AssemblyHelpers.h:934
>>> +    Jump branchIfBigInt32(GPRReg gpr, GPRReg tempGPR)
>> 
>> I would extend it further by using number-tag-register later.
>> 
>> if (mode == HaveTagRegisters) {
>> move(GPRInfo::numberTagRegister, tempGPR);
>> add64(TrustedImm32(JSValue::BigInt32Tag), tempGPR);
>> and64(gpr, tempGPR);
>> return branch64(Equal, tempGPR, TrustedImm32(JSValue::BigInt32Tag));
>> }
> 
> Or further,
> 
> if (mode == HaveTagRegisters) {
>     add64(TrustedImm32(JSValue::BigInt32Tag), GPRInfo::numberTagRegister, tempGPR);
>     and64(gpr, tempGPR);
>     return branch64(Equal, tempGPR, TrustedImm32(JSValue::BigInt32Tag));
> }

Let's also static assert that this produce the same value in C++
Comment 10 Yusuke Suzuki 2020-04-22 13:56:56 PDT
Created attachment 397255 [details]
Patch
Comment 11 Yusuke Suzuki 2020-04-22 14:09:32 PDT
Created attachment 397259 [details]
Patch
Comment 12 Yusuke Suzuki 2020-04-22 15:07:24 PDT
Created attachment 397271 [details]
Patch
Comment 13 Yusuke Suzuki 2020-04-22 20:11:08 PDT
Committed r260551: <https://trac.webkit.org/changeset/260551>
Comment 14 Radar WebKit Bug Importer 2020-04-22 20:12:20 PDT
<rdar://problem/62228618>