Bug 238018 – [JSC] Add DoNotHaveTagRegisters mode to unboxDouble

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

[Xan Lopez]

This allows us to use it in WebAssembly and get rid of some duplicated code.

[Xan Lopez]

Created attachment 454950 [details]
v1

[Radar WebKit Bug Importer]

<rdar://problem/90639371>

[Yusuke Suzuki]

Comment on attachment 454950 [details]
v1

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

r=me with comment.

> Source/JavaScriptCore/jit/AssemblyHelpers.h:1386
> +            GPRReg scratch = scratchRegister();
> +            move(TrustedImm64(JSValue::NumberTag), scratch);
> +            add64(scratch, gpr, resultGPR);

I think we should not use scratchRegister() in AssemblyHelpers if possible.
Can you instead add,

void MacroAssembler::add64(TrustedImm64, GPRReg, GPRReg)

function and use it instead here? And inside add64, we can use scratchRegister.

[Xan Lopez]

Created attachment 455489 [details]
v2

[EWS]

Committed r291745 (248777@main): <https://commits.webkit.org/248777@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455489 [details].

[Yusuke Suzuki]

Caused several JSC debug test failures.
https://results.webkit.org/?suite=javascriptcore-tests&test=wasm.yaml%2Fwasm%2Ffunction-tests%2Ffunction-import-return-value.js.default-wasm

[WebKit Commit Bot]

Re-opened since this is blocked by bug 238399

[Yusuke Suzuki]

Comment on attachment 455489 [details]
v2

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

> Source/JavaScriptCore/wasm/js/WasmToJS.cpp:331
> -            jit.move(JIT::TrustedImm64(JSValue::NumberTag), GPRInfo::returnValueGPR2);
> -            jit.add64(GPRInfo::returnValueGPR2, GPRInfo::returnValueGPR);
> -            jit.move64ToDouble(GPRInfo::returnValueGPR, dest);
> +            jit.unboxDouble(GPRInfo::returnValueGPR, GPRInfo::returnValueGPR2, dest, DoNotHaveTagRegisters);

At this point, we cannot use scratchRegister().

[Xan Lopez]

Created attachment 456676 [details]
v3

Do not use a scratch register as suggested. This was not enough to get rid of the crash, it turned out the assertion in the vanilla unboxDouble call was at fault. So I'm using the WithoutAssertions call, which makes this patch 100% equivalent to the old code (literally just moving the identical lines to a common method). Not sure if the extra assertion is uncovering a bug/issue or if we were being too strict without motive, though.

[Yusuke Suzuki]

Comment on attachment 456676 [details]
v3

r=me

[EWS]

Committed r292457 (249308@main): <https://commits.webkit.org/249308@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456676 [details].