How to distinguish v8 target arch in torque builtins?

[Zhao Jiazhong]

Hi all,

I'm porting JSPI to loong64 port, and find an issue that in JSToWasmWrapperHelper, a 32-bit value is converted to unsigned then converted to intptr, which leading to a zero-extended value, but on LoongArch64, we need the 32-bit value to be sign-extended in 64-bit registers.

I don't want to change the behavior on other arches, but the builtin is written in torque, I suppose I can't use `V8_TARGET_ARCH_XXX` macro in it, so is there a way to distinguish v8 target arch in torque builtins? Thanks!

Yours,

Zhao Jiazhong

[Nico Hartmann]

Hi Zhao,

you can use Torque's @if and @ifnot annotations to make such distinctions (check @if(TAGGED_SIZE_8_BYTES) for an example). You then need to set this from the C++ side in torque-parser.cc and for that you can use the usual `V8_TARGET_ARCH_XXX`. The places where you can use such annotations are a bit restricted if I remember correctly, but it should be enough to support your case (maybe see this for an example). Hope that helps.

Cheers

Nico

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/c2790cc7-513d-4296-8531-f620fe93e038n%40googlegroups.com.

--

Nico Hartmann | Software Engineer | nicoha...@google.com | Chrome - V8

[Zhao Jiazhong]

Hi Nico,

Thanks for your information, it indeed works!

But it seems that the @if and @ifnot annotations can't handle two conditions at once like `@if(V8_TARGET_ARCH_LOONG64 || V8_TARGET_ARCH_MIPS64)`, and I didn't find something like `@elif`.

I thinks MIPS64 is likely need this change too, but add @if(MIPS64) and @ifnot(MIPS64) in @ifnot(LOONG64) seems ugly. Do you have any suggestions? Thank you very much!

Yours,
Zhao Jiazhong

[Nico Hartmann]

I would give this "condition" a name

`@if(NEEDS_SPECIAL_INT32_TO_INT64_SIGN_EXTENSION)`

(maybe find something more descriptive) and then set this build flag for all architectures that need that.

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/88402770-d013-4847-8688-cfdf6cf0793an%40googlegroups.com.

[Leszek Swirski]

Why is this different on LoongArch64 to other platforms anyway? Could we sign extend this on all platforms rather than conditionally? cc @Andreas Haas 

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAHNjccKGuJRZ5d21%2B06YiSWjc6CaERZs0SC6cBA-Lq1kLgKQcw%40mail.gmail.com.

[Zhao Jiazhong]

It's a good idea, I will use this method. Thanks!

[Andreas Haas]

I think it should not be a problem to remove the `Unsigned` there for other platforms as well, if it works. What's happening there is that a Smi gets converted into an int32 and then stored in a 64-bit slot on the stack. If the second half of the 64-bit slot gets filled with ones or with zeroes does not matter, I think. If it matters, then there are tests in place.

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/3e4f56ab-a3ab-4e12-941b-9319c2e29a68n%40googlegroups.com.

--

Andreas Haas

Software Engineer

ah...@google.com

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde.

    

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.

[Zhao Jiazhong]

Some tests like `wasm-spec-tests/address` failed on x64 if remove the `Unsigned`.

[Andreas Haas]

But will these tests not also fail on loong64 then? Why do you expect different behavior there?

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/06ce8d10-ee9a-4993-946e-1c6bc3e352cdn%40googlegroups.com.

[Zhao Jiazhong]

Losts of tests failed on loong64 with the `Unsigned`, and they would pass if I remove the `Unsigned` for loong64.

It's because LoongArch64, MIPS64 and perhaps RISCV64 all generally need to sign-extend 32-bit values in 64-bit registers according to their calling convention. But the convert to unsigned then to intptr will generate a zero-extended 32-bit value, which caused the failue on loong64 port. Not sure why x64 needs the `Unsigned`.

[Andreas Haas]

The code there converts an incoming Smi parameter to an int32 and writes that int32 into a buffer on the stack. This buffer is then read in an assembly builtin [1], where the int32 is loaded either into a register or pushed on the stack. I think you actually have a bug in the assembly builtin, not in the Torque builtin. 

There is a fuzzer-like mjsunit test [2] that allows you to test this code very well. If you set the `debug` variable at the beginning of the test to true, you enable the output and increase the number of test cases. It's very helpful to debug your implementation.

Cheers, Andreas

[1] https://source.chromium.org/chromium/chromium/src/+/main:v8/src/builtins/loong64/builtins-loong64.cc;l=3037;drc=9b2c99f8734501549f5726bc187d3f05dfaa7af0

[2] https://source.chromium.org/chromium/chromium/src/+/main:v8/test/mjsunit/wasm/torque-wrapper.js;l=25;drc=b2df50672c06c070ecb137570d1072a3835ac4ca

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/6d148e25-963e-4c60-b7b5-5f7c11c64e2fn%40googlegroups.com.

[Zhao Jiazhong]

Okay, I will have a look at the assembly builtin. Thanks for your detailed information!

[Zhao Jiazhong]

I still think loong64 port needs to modify the torque builtin.

I ported JSPI to loong64 according to arm64 port, and it seems the assembly builtin just load the parameters by full 64 bits and pass them to JIT code? I wonder if the builtin have the information about the bit width of the arguments.

I uploaded my patch to gerrit: [loong64][wasm][jspi] Initial port of JSPI to loongarch64 architecture, welcome to review. Thanks!

[Andreas Haas]

No, the assembly builtin loads the value as it is from the buffer on the stack into the registers, it does not know if it is loading a 32-bit value or a 64-bit value.

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/7b281bc6-f010-4e06-9f10-60f74163e010n%40googlegroups.com.

[Thibaud Michaud]

A quick update from an offline conversation with Andreas:

It looks like we already clear the upper half of i32 register params in the function prologues almost everywhere, so the unsigned conversion in the generic js-to-wasm wrapper does look redundant.

The only case that is missing is on no-sandbox builds, specifically in Liftoff, because so far this was only done for sandbox-related reasons.

It seems fine to extend that to all configs, that would be more consistent and would allow us to keep the torque code platform-independent.

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/CAELSTvfHEeOkJ2kotTyrJk9djm48nRUYLnWHCn1jipd0ZVHyJg%40mail.gmail.com.

[Zhao Jiazhong]

After Thibaud's CL, the loong64 port works fine now without arch-specific changes to the torque builtin.

Thanks for everyone's kindly help!