wasmtime / PR #5017 Tls and abi for riscv64. · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #5017 Tls and abi for riscv64.

Wasmtime GitHub notifications bot (Oct 05 2022 at 01:50):

yuyang-ok opened PR #5017 from tls_and_abi to main:

This is try to fix https://github.com/bytecodealliance/wasmtime/issues/4994 and https://github.com/bytecodealliance/wasmtime/issues/5008.

But still working.

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

bjorn3 created PR review comment:

This should take the clobber set from the C call conv (and remove a0)

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

bjorn3 created PR review comment:

Should this also be a fixed use?

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

bjorn3 submitted PR review.

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

bjorn3 submitted PR review.

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

yuyang-ok updated PR #5017 from tls_and_abi to main.

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

yuyang-ok submitted PR review.

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

yuyang-ok created PR review comment:

@bjorn3 , I forget this.

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

yuyang-ok updated PR #5017 from tls_and_abi to main.

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

yuyang-ok submitted PR review.

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

yuyang-ok created PR review comment:

This should do at riscv64_get_operands.

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

yuyang-ok has marked PR #5017 as ready for review.

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

yuyang-ok requested bjorn3 for a review on PR #5017.

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

yuyang-ok updated PR #5017 from tls_and_abi to main.

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

yuyang-ok updated PR #5017 from tls_and_abi to main.

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

yuyang-ok updated PR #5017 from tls_and_abi to main.

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

yuyang-ok updated PR #5017 from tls_and_abi to main.

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

yuyang-ok updated PR #5017 from tls_and_abi to main.

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

yuyang-ok updated PR #5017 from tls_and_abi to main.

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

yuyang-ok updated PR #5017 from tls_and_abi to main.

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

yuyang-ok updated PR #5017 from tls_and_abi to main.

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

afonso360 submitted PR review.

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

afonso360 created PR review comment:

It would be nice if we could add some compile tests for this

Wasmtime GitHub notifications bot (Oct 06 2022 at 00:10):

yuyang-ok updated PR #5017 from tls_and_abi to main.

Wasmtime GitHub notifications bot (Oct 06 2022 at 00:11):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2022 at 00:11):

yuyang-ok created PR review comment:

Yes,I have add one test just like other platform, But that one test doesn't mean a lot to me.

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

yuyang-ok updated PR #5017 from tls_and_abi to main.

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

yuyang-ok submitted PR review.

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

yuyang-ok created PR review comment:

I already have the float abi and tls tested.

Wasmtime GitHub notifications bot (Oct 08 2022 at 17:26):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 08 2022 at 17:26):

bjorn3 created PR review comment:

The relocations here don't seem to be correct. You first need an auipc with R_RISCV_TLS_GD_HI20 and then an addi with R_RISCV_PCREL_LO12_I. You only have the first one.

Wasmtime GitHub notifications bot (Oct 08 2022 at 17:26):

bjorn3 created PR review comment:

nit:

                        // We are already done with register alloc.

Wasmtime GitHub notifications bot (Oct 08 2022 at 17:26):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 08 2022 at 17:27):

bjorn3 created PR review comment:

Could you also add a use for a0?

Wasmtime GitHub notifications bot (Oct 08 2022 at 17:27):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 08 2022 at 17:28):

bjorn3 created PR review comment:

Support for this needs to be implemented in cranelift_object. Should be pretty similar to https://github.com/bytecodealliance/wasmtime/pull/4995/files#diff-b825ad4c01be6ae3d42dd6ace012f2815e5e52458abc4f436526a5c4aaa769adR711

Wasmtime GitHub notifications bot (Oct 08 2022 at 17:28):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 08 2022 at 18:35):

cfallin created PR review comment:

@bjorn3 to clarify, what would the use be for? The instruction only has one register arg (rd), which is output-only; does something else flow in?

Wasmtime GitHub notifications bot (Oct 08 2022 at 18:35):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 08 2022 at 18:39):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 08 2022 at 18:39):

bjorn3 created PR review comment:

Right, the call contained within it uses a0, but that one is defined earlier in this macro instruction.

Wasmtime GitHub notifications bot (Oct 09 2022 at 01:17):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 09 2022 at 01:17):

cfallin created PR review comment:

Yes, and given that, a0 is not a use of the pseudoinstruction: it is not live-in, and the register allocator does not need to track a liverange for an input that ends up in a0. Purely internal operations can read and write a0 as many times as they like, since the def constrained to a0 allows the instruction to have an overall effect of "overwrites a0 with output value resulting".

So, @yuyang-ok: this code is correct as-is, no worries!

Wasmtime GitHub notifications bot (Oct 09 2022 at 01:22):

yuyang-ok created PR review comment:

ok.

Wasmtime GitHub notifications bot (Oct 09 2022 at 01:22):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Oct 09 2022 at 01:22):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Oct 09 2022 at 01:22):

yuyang-ok created PR review comment:

sorry.

Wasmtime GitHub notifications bot (Oct 09 2022 at 01:23):

yuyang-ok created PR review comment:

Yes ,a0 is aslo used.

Wasmtime GitHub notifications bot (Oct 09 2022 at 01:23):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Oct 09 2022 at 01:23):

yuyang-ok edited PR review comment.

Wasmtime GitHub notifications bot (Oct 09 2022 at 01:24):

yuyang-ok created PR review comment:

@bjorn3 Do you mean the name Reloc::RiscvCall should be Reloc::R_RISCV_CALL????

Wasmtime GitHub notifications bot (Oct 09 2022 at 01:24):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Oct 09 2022 at 02:16):

yuyang-ok deleted PR review comment.

Wasmtime GitHub notifications bot (Oct 09 2022 at 02:17):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Oct 09 2022 at 02:17):

yuyang-ok created PR review comment:

@cfallin Yes, no need use,aarch64 did the same.

Wasmtime GitHub notifications bot (Oct 09 2022 at 02:30):

yuyang-ok updated PR #5017 from tls_and_abi to main.

Wasmtime GitHub notifications bot (Oct 09 2022 at 11:47):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 09 2022 at 11:47):

bjorn3 created PR review comment:

This is what I meant:

diff --git a/cranelift/object/src/backend.rs b/cranelift/object/src/backend.rs
index f40a75822..c06c2e3e4 100644
--- a/cranelift/object/src/backend.rs
+++ b/cranelift/object/src/backend.rs
@@ -720,6 +720,32 @@ impl ObjectModule {
                     0,
                 )
             }
+            Reloc::RiscvTlsGdHi20 => {
+                assert_eq!(
+                    self.object.format(),
+                    object::BinaryFormat::Elf,
+                    "RiscvTlsGdHi20 is not supported for this file format"
+                );
+                (
+                    RelocationKind::Elf(object::elf::R_RISCV_TLS_GD_HI20),
+                    RelocationEncoding::Generic,
+                    0,
+                )
+
+            }
+            Reloc::RiscvPCRelLo12I => {
+                assert_eq!(
+                    self.object.format(),
+                    object::BinaryFormat::Elf,
+                    "RiscvPCRelLo12I is not supported for this file format"
+                );
+                (
+                    RelocationKind::Elf(object::elf::R_RISCV_PCREL_LO12_I),
+                    RelocationEncoding::Generic,
+                    0,
+                )
+
+            }
             // FIXME
             reloc => unimplemented!("{:?}", reloc),
         };

Wasmtime GitHub notifications bot (Oct 09 2022 at 11:49):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 09 2022 at 11:49):

bjorn3 created PR review comment:

So I just tried this and the linker gave an error. When I tried comparing it with what LLVM produces I found that this relocation needs to have a label pointing to the previous instruction as target: https://godbolt.org/z/8P31Gcc7Y @cfallin how should an api for allowing backends to produce this kind of labels look like?

Wasmtime GitHub notifications bot (Oct 09 2022 at 23:45):

yuyang-ok deleted PR review comment.

Wasmtime GitHub notifications bot (Oct 10 2022 at 01:29):

yuyang-ok created PR review comment:

@bjorn3 In that case I think one reloc maybe be more reasonable like ElfTlsGD,ElfTlsGD means

.LBB0_1:
auipc a0, %tls_gd_pcrel_hi(example::FOO)
addi a0, a0, %pcrel_lo(.LBB0_1)

the whole bunch of things.
`ElfTlsGD+0` means the label and tls_gd_pcrel_hi, `ElfTlsGD + 4` means the pcrel_lo.

Wasmtime GitHub notifications bot (Oct 10 2022 at 01:29):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2022 at 07:54):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2022 at 07:54):

yuyang-ok created PR review comment:

ok.

Wasmtime GitHub notifications bot (Oct 10 2022 at 13:01):

bjorn3 created PR review comment:

https://github.com/llvm/llvm-project/blob/9c626d4a0d4d75c976c9a7c9093ea885fcbd98d5/lld/ELF/InputSection.cpp#L542-L580

It doesn't seem to work that way it seems. For R_RISCV_PCREL_LO12_{I,S} the target is always a label pointing to a place with a R_RISCV_PCREL_HI20, R_RISCV_GOT_HI20, R_RISCV_TLS_GD_HI20 or R_RISCV_TLS_GOT_HI20. The actual symbol for which the R_RISCV_PCREL_LO12_{I,S} works on is taken from this relocation, rather than from the R_RISCV_PCREL_LO12_{I,S} itself. In other words it uses example::FOO as symbol rather than .LBB0_1.

Wasmtime GitHub notifications bot (Oct 10 2022 at 13:01):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2022 at 20:00):

cfallin created PR review comment:

@cfallin how should an api for allowing backends to produce this kind of labels look like?

In this case the MachInst should generate both instructions I think (auipc and addi); that way the label use is internal to the MachInst. Anything more complex than that needs a lot of additional plumbing that we don't have...

Wasmtime GitHub notifications bot (Oct 10 2022 at 20:00):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2022 at 20:01):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 10 2022 at 20:01):

bjorn3 created PR review comment:

Yeah, the label needs to be exported as entry in the ELF symbol table.

Wasmtime GitHub notifications bot (Oct 11 2022 at 23:12):

yuyang-ok updated PR #5017 from tls_and_abi to main.

Wasmtime GitHub notifications bot (Nov 17 2022 at 03:24):

yuyang-ok updated PR #5017 from tls_and_abi to main.

Wasmtime GitHub notifications bot (Oct 06 2023 at 13:55):

alexcrichton closed without merge PR #5017.

Last updated: Oct 23 2024 at 20:03 UTC