Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does cranelift support the f16const in riscv64? #9508

Open
abc767234318 opened this issue Oct 24, 2024 · 11 comments
Open

Does cranelift support the f16const in riscv64? #9508

abc767234318 opened this issue Oct 24, 2024 · 11 comments

Comments

@abc767234318
Copy link

I constructed a clif file that contains the f16const instruction, and I got an error when I used the following command.

qemu-riscv64 -L /usr/riscv64-linux-gnu/ -E LD_LIBRARY_PATH=/usr/riscv64-linux-gnu/lib  clif-util test test.clif

The error is:

 ERROR cranelift_filetests::concurrent > FAIL: failed to parse test.clif
FAIL test.clif: failed to parse test.clif

Caused by:
    16: Unknown opcode: 'f16const'
1 tests
Error: 1 failure

In addition, I found a test file for the f16const instruction in the cranelift/filetests/filetests/isa/riscv64/f16const.clif.

@cfallin
Copy link
Member

cfallin commented Oct 24, 2024

Interesting -- it seems we should, since it's in a test and tests are passing on CI.

A few things to verify:

  • Can you check that your version of clif-util is the latest, i.e. built from current main?
  • Can you check if you're able to run clif-util test on cranelift/filetests/filetests/isa/riscv64/f16const.clif?
  • If things are still not working, can you share the full CLIF input file, exact version (commit) and command line you're running?

@abc767234318
Copy link
Author

Interesting -- it seems we should, since it's in a test and tests are passing on CI.

A few things to verify:

  • Can you check that your version of clif-util is the latest, i.e. built from current main?
  • Can you check if you're able to run clif-util test on cranelift/filetests/filetests/isa/riscv64/f16const.clif?
  • If things are still not working, can you share the full CLIF input file, exact version (commit) and command line you're running?

It seems that my version of wasmtime is wrong, I will close this issue.

@abc767234318
Copy link
Author

@cfallin Here is my CLIF input file.
output.zip

However, I still can't run this file by using the following command line:

qemu-riscv64 -L /usr/riscv64-linux-gnu -E LD_LIBRARY_PATH=/usr/riscv64-linux-gnu/lib riscv64gc-unknown-linux-gnu/debug/clif-util run -v output.clif

The error is :

output.clif: The target ISA specified in the file is not compatible with the host ISA
1 file
Error: 1 failure

And I try to run the f16const.clif, it also outputs the similar error:

qemu-riscv64 -L /usr/riscv64-linux-gnu -E LD_LIBRARY_PATH=/usr/riscv64-linux-gnu/lib riscv64gc-unknown-linux-gnu/debug/clif-util run 
-v ../cranelift/filetests/filetests/isa/riscv64/f16const.clif 
../cranelift/filetests/filetests/isa/riscv64/f16const.clif: The target ISA specified in the file is not compatible with the host ISA
1 file
Error: 1 failure

The commit version of my wasmtime is:

~/wasmtime$ git rev-parse HEAD
c255a853f47188f43fb004389581fccc74bfc362

@abc767234318 abc767234318 reopened this Oct 24, 2024
@bjorn3
Copy link
Contributor

bjorn3 commented Oct 24, 2024

You are probably missing target riscv64 at the top of the test file (didn't download your zip).

@afonso360
Copy link
Contributor

afonso360 commented Oct 24, 2024

FP16 on the RISC-V backend is only supported with the Zfh extension. You need to add the has_zfh flag to the target field. (i.e. target riscv64 has_zfh)

You also have to specify support for the Zfh extension in qemu otherwise it likely won't run the test. There's a flag somewhere to do it, I don't know exactly what it is.

I should also note, there are some operations that we don't yet support for FP16.

@bjorn3
Copy link
Contributor

bjorn3 commented Oct 24, 2024

FP16 on the RISC-V backend is only supported with the Zfh extension.

For cg_clif f16const and f16 arguments and return values will need to work unconditionally. Math on f16 can be lowered to libcalls just fine when Zfh is not enabled however.

@afonso360
Copy link
Contributor

I'm not entirely sure how it's currently implemented, but I think that should work. We already save and restore the entire registers, instead of just the used portion. I haven't tested that yet though.

@afonso360
Copy link
Contributor

@abc767234318 I'm not entirely sure why the clif-util run command doesn't work, but running the test with clif-util test does work on my machine.

I also tried to run clif-util compile --target riscv64 --set has_zfh output.clif, but that failed because we don't support f128 types.

@abc767234318
Copy link
Author

@abc767234318 I'm not entirely sure why the clif-util run command doesn't work, but running the test with clif-util test does work on my machine.

I also tried to run clif-util compile --target riscv64 --set has_zfh output.clif, but that failed because we don't support f128 types.

Where can I find the set of IR instructions supported by each cpu architecture? I also found that x86 cpu don't seem to support the iconst.i128 instruction.

The line 13 is v7 = iconst.i128 -2319861032952027390, but I get the following error

 ERROR cranelift_filetests::concurrent > FAIL: failed to parse file_tests/multi_func.clif
FAIL file_tests/multi_func.clif: failed to parse file_tests/multi_func.clif

Caused by:
    13: expected one of the following type: i8, i16, i32 or i64
1 tests
Error: 1 failure

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 20, 2024

iconst.i128 is not allowed on any architecture afaik. You have to use two iconst.i64 + an iconcat with both halves to get an i128. The iconst instruction only accepts a 64bit immediate value.

@cfallin
Copy link
Member

cfallin commented Nov 20, 2024

To add a bit more: we don't support iconst.i128 because it would imply that the size of an InstructionData is at least 128 bits (16 bytes) plus the opcode; whereas we try to keep the entire InstructionData to 16 bytes for performance and memory-overhead reasons. The tradeoff is that the one instruction that has a huge immediate (iconst.i128) is not possible to encode directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants