Bug 210870

Summary: [JSC] branchIfBigInt32 can use BigInt32Mask and remove branchIfNumber filter
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, rmorisset, sbarati, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch sbarati: review+

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>