Stream: git-wasmtime

Topic: wasmtime / issue #4994 Riscv float abi variant handling

Wasmtime GitHub notifications bot (Oct 02 2022 at 15:53):

bjorn3 opened issue #4994:

Riscv has different abi variants which pass floating point arguments differently. This is controlled independently of the used target features and the linker checks that object files with different abi variants are not linked together. Currently Cranelift doesn't handle this at all and effectively uses the double float abi unconditionally. We will need a way to specify the float abi (soft, single, double), use this float abi even if the target features would allow the double float abi and expose a way for cranelift-object to get the chosen float abi to put in the e_flags field of an elf binary.

Wasmtime GitHub notifications bot (Oct 05 2022 at 02:23):

yuyang-ok commented on issue #4994:

• /// SystemV riscv64 have different ABI variants.

• SystemVRiscv(SystemVRiscv),
  +}
  +
  +pub enum SystemVRiscv {

• ILP32,

• ILP32F,
• ILP32D,
• LP64,
• LP64F,
• LP64D,

• LP64Q,
  +}
  +
  +impl SystemVRiscv {

• pub fn is_32(self) -> bool {

• match self {
• Self::ILP32 | Self::ILP32F | Self::ILP32D => true,
• _ => false,
• }
• }
• pub fn is_64() -> bool {
• match self {
• Self::LP64 | Self::LP64F | Self::LP64D | Self::LP64Q => true,
• _ => false,
• }
• }
  }

impl CallConv {
yuyang@yuyang-Inspiron-7590:~/projects/wasmtime/cranelift$

@bjorn3   I want to add these callconv, looks ok??

Wasmtime GitHub notifications bot (Oct 05 2022 at 05:04):

yuyang-ok deleted a comment on issue #4994:

• /// SystemV riscv64 have different ABI variants.

• SystemVRiscv(SystemVRiscv),
  +}
  +
  +pub enum SystemVRiscv {

• ILP32,

• ILP32F,
• ILP32D,
• LP64,
• LP64F,
• LP64D,

• LP64Q,
  +}
  +
  +impl SystemVRiscv {

• pub fn is_32(self) -> bool {

• match self {
• Self::ILP32 | Self::ILP32F | Self::ILP32D => true,
• _ => false,
• }
• }
• pub fn is_64() -> bool {
• match self {
• Self::LP64 | Self::LP64F | Self::LP64D | Self::LP64Q => true,
• _ => false,
• }
• }
  }

impl CallConv {
yuyang@yuyang-Inspiron-7590:~/projects/wasmtime/cranelift$

@bjorn3   I want to add these callconv, looks ok??

Wasmtime GitHub notifications bot (Oct 05 2022 at 05:11):

yuyang-ok commented on issue #4994:

• /// SystemV riscv64 have different ABI variants.

• SystemVRiscv(SystemVRiscv),
  +}
  +
  +pub struct SystemVRiscv {

• float_call_conv: RiscvFloatCallConv,
  +}
  +
  +pub enum RiscvFloatCallConv {

• Soft,

• Single,
• Double,

• Quad,
  +}
  +
  +impl SystemVRiscv {

• pub fn is_32(self) -> bool {

• match self {
• Self::ILP32 | Self::ILP32F | Self::ILP32D => true,
• _ => false,
• }
• }
• pub fn is_64() -> bool {
• match self {
• Self::LP64 | Self::LP64F | Self::LP64D | Self::LP64Q => true,
• _ => false,
• }
• }
  }

@bjorn3  looks fine???

Wasmtime GitHub notifications bot (Oct 05 2022 at 05:12):

yuyang-ok edited a comment on issue #4994:

 WasmtimeAppleAarch64,

• /// SystemV riscv have different ABI variants.

• SystemVRiscv(SystemVRiscv),
  +}
  +
  +pub struct SystemVRiscv {

• float_call_conv: RiscvFloatCallConv,
  +}
  +
  +pub enum RiscvFloatCallConv {

• Soft,

• Single,
• Double,
• Quad,
  }

@bjorn3  looks fine???

Wasmtime GitHub notifications bot (Oct 05 2022 at 06:23):

bjorn3 commented on issue #4994:

That should work I think. I do think there is still a need for an isa flag to specify which float call conv should be returned by the default_call_conv field.

Wasmtime GitHub notifications bot (Oct 05 2022 at 08:53):

yuyang-ok commented on issue #4994:

I do think there is still a need for an isa flag to specify which float call conv should be returned by the default_call_conv field.

I am not understand this. I didn't see any connection between isa flags and default_call_conv.

Wasmtime GitHub notifications bot (Oct 05 2022 at 09:07):

bjorn3 commented on issue #4994:

The TargetIsa has a frontend_config method, which returns a value with the default_call_conv field. To make this method return the right value in the default_call_conv field, there has to be an isa flag to choose it.

Wasmtime GitHub notifications bot (Oct 05 2022 at 10:57):

yuyang-ok commented on issue #4994:

@bjorn3 Sorry, I must missed that, I have add the setting.

Wasmtime GitHub notifications bot (Oct 05 2022 at 11:14):

yuyang-ok commented on issue #4994:

Running tests/filetests.rs (/home/yuyang/projects/wasmtime/target/riscv64gc-unknown-linux-gnu/debug/deps/filetests-ac71876a5d2b4558)
thread 'worker #7' panicked at 'not implemented: ISA flag float_abi of kind Enum', cranelift/filetests/src/test_run.rs:117:13
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace
thread 'worker #8' panicked at 'not implemented: ISA flag float_abi of kind Enum', cranelift/filetests/src/test_run.rs:117:13
thread 'worker #6' panicked at 'not implemented: ISA flag float_abi of kind Enum', cranelift/filetests/src/test_run.rs:117:13
thread 'worker #9' panicked at 'not implemented: ISA flag float_abi of kind Enum', cranelift/filetests/src/test_run.rs:117:13
thread 'worker #6' panicked at 'not implemented: ISA flag float_abi of kind Enum', cranelift/filetests/src/test_run.rs:117:13
thread 'worker #7' panicked at 'not implemented: ISA flag float_abi of kind Enum', cranelift/filetests/src/test_run.rs:117:13
thread 'worker #6' panicked at 'not implemented: ISA flag float_abi of kind Enum', cranelift/filetests/src/test_run.rs:117:13
thread 'worker #8' panicked at 'not implemented: ISA flag float_abi of kind Enum', cranelift/filetests/src/test_run.rs:117:13
thread 'worker #7' panicked at 'not implemented: ISA flag float_abi of kind Enum', cranelift/filetests/src/test_run.rs:

I am getting this error.

Wasmtime GitHub notifications bot (Oct 05 2022 at 11:16):

bjorn3 commented on issue #4994:

Not sure how to deal with that.

Wasmtime GitHub notifications bot (Oct 05 2022 at 11:16):

bjorn3 commented on issue #4994:

Not sure how to deal with that.

Wasmtime GitHub notifications bot (Oct 05 2022 at 11:17):

bjorn3 deleted a comment on issue #4994:

Not sure how to deal with that.

Wasmtime GitHub notifications bot (Oct 05 2022 at 11:31):

yuyang-ok commented on issue #4994:

This is indeed not unimplemented.:-)

Wasmtime GitHub notifications bot (Oct 05 2022 at 11:31):

yuyang-ok edited a comment on issue #4994:

This is indeed unimplemented.:-)

Wasmtime GitHub notifications bot (Oct 05 2022 at 11:57):

afonso360 commented on issue #4994:

The runtest suite tries to check if the hardware supports the flags that were requested for this test.

This is done so that if a particular test tries to compile the code with has_avx512 and the host machine does not support avx512 we don't try to run the code and cause an illegal instruction error.

Currently that code only supports boolean values, I don't mind extending it to also support enums, but that would also mean that we have to choose some variant of the riscv float abi to be the one that is detected by cranelift-native and tests would only be able to use that one in particular or they would be skipped.

That variant also has to be the default one, or we skip all riscv tests.

Wasmtime GitHub notifications bot (Oct 05 2022 at 12:16):

afonso360 edited a comment on issue #4994:

The runtest suite tries to check if the hardware supports the flags that were requested for this test.

This is done so that if a particular test tries to compile the code with has_avx512 and the host machine does not support avx512 we don't try to run the code and cause an illegal instruction error.

Currently that code only supports boolean values, I don't mind extending it to also support enums, ~but that would also mean that we have to choose some variant of the riscv float abi to be the one that is detected by cranelift-native and tests would only be able to use that one in particular or they would be skipped.~

~That variant also has to be the default one, or we skip all riscv tests.~

Edit: I think there is probably a better way to do this, but for now allowing all float_abi's seems okay

Wasmtime GitHub notifications bot (Oct 05 2022 at 22:39):

yuyang-ok commented on issue #4994:

https://bytecodealliance.zulipchat.com/#narrow/stream/206238-general/topic/add_enum.20can't.20have.20default.20value.3F.3F

Wasmtime GitHub notifications bot (Oct 13 2022 at 05:56):

yuyang-ok deleted a comment on issue #4994:

https://bytecodealliance.zulipchat.com/#narrow/stream/206238-general/topic/add_enum.20can't.20have.20default.20value.3F.3F

Last updated: Oct 23 2024 at 20:03 UTC