Stream: git-wasmtime

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


view this post on Zulip 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.

view this post on Zulip 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)

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 06:30):

bjorn3 created PR review comment:

Should this also be a fixed use?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 06:30):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 06:33):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 07:42):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 07:44):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 07:44):

yuyang-ok created PR review comment:

@bjorn3 , I forget this.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 07:57):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 07:58):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 07:58):

yuyang-ok created PR review comment:

This should do at riscv64_get_operands.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 08:02):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 08:09):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 08:14):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 08:15):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 08:30):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 08:38):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 10:40):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 10:42):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 10:56):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 11:51):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 05 2022 at 12:31):

afonso360 submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2022 at 00:10):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2022 at 00:11):

yuyang-ok submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2022 at 05:14):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 07 2022 at 05:14):

yuyang-ok submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 08 2022 at 17:26):

bjorn3 submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 08 2022 at 17:26):

bjorn3 created PR review comment:

nit:

                        // We are already done with register alloc.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 08 2022 at 17:26):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 08 2022 at 17:27):

bjorn3 created PR review comment:

Could you also add a use for a0?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 08 2022 at 17:27):

bjorn3 submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 08 2022 at 17:28):

bjorn3 submitted PR review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 08 2022 at 18:35):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 08 2022 at 18:39):

bjorn3 submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 01:17):

cfallin submitted PR review.

view this post on Zulip 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!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 01:22):

yuyang-ok created PR review comment:

ok.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 01:22):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 01:22):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 01:22):

yuyang-ok created PR review comment:

sorry.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 01:23):

yuyang-ok created PR review comment:

Yes ,a0 is aslo used.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 01:23):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 01:23):

yuyang-ok edited PR review comment.

view this post on Zulip 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????

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 01:24):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 02:16):

yuyang-ok deleted PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 02:17):

yuyang-ok submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 02:30):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 11:47):

bjorn3 submitted PR review.

view this post on Zulip 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),
         };

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 11:49):

bjorn3 submitted PR review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 09 2022 at 23:45):

yuyang-ok deleted PR review comment.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 10 2022 at 01:29):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 10 2022 at 07:54):

yuyang-ok submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 10 2022 at 07:54):

yuyang-ok created PR review comment:

ok.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 10 2022 at 13:01):

bjorn3 submitted PR review.

view this post on Zulip 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...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 10 2022 at 20:00):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 10 2022 at 20:01):

bjorn3 submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 11 2022 at 23:12):

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

view this post on Zulip Wasmtime GitHub notifications bot (Nov 17 2022 at 03:24):

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

view this post on Zulip Wasmtime GitHub notifications bot (Oct 06 2023 at 13:55):

alexcrichton closed without merge PR #5017.


Last updated: Dec 23 2024 at 12:05 UTC