wasmtime / PR #4271 add riscv64 backend for cranelift. · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #4271 add riscv64 backend for cranelift.

Wasmtime GitHub notifications bot (Jun 15 2022 at 05:58):

yuyang-ok opened PR #4271 from risc-v to main:

I am been trying to add riscv64 backend for cranelift these days.
right now I have pass all run test in filetests.

some features not implemented right now.
i128 mul div rem, all simd type and compare overflow.

some test need platform support.
like bitrev need qemu-system-riscv64 support bitmanip and zbkb extension (don't know how to enable it.).

Wasmtime GitHub notifications bot (Jun 15 2022 at 06:00):

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

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 created PR review comment:

Just has_m is fine I think.

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 created PR review comment:

This should also accept riscv64imac I think. This is the rustc target for 64bit riscv without the F, D, Zicsr, and Zifencei features.

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 created PR review comment:

Maybe add presets for riscv64imac and riscv64gc? The x86_64 backend uses code like the following: https://github.com/bytecodealliance/wasmtime/blob/fd3ce1a6cb47b60440d5d35b0670e9139ccd8eb5/cranelift/codegen/meta/src/isa/x86.rs#L185-L189

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 created PR review comment:

nit:
riscv64gc = []
to keep visual separation.

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 created PR review comment:

This list should also include has_c.

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 created PR review comment:

Can you move this to just below Self::Arm32Call | Self::Arm64Call => write!(f, "Call") and keep the newline below that? The newline separates all TLS relocations from the rest. Also please capitalize as RiscvCall for consistency.

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 created PR review comment:

There are several unrelated changes across this PR. This is one of them. The change to tests/spec_testsuite is another. Could you please revert them?

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 created PR review comment:

This will need to be conditional on riscv64 being used.

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 created PR review comment:

Please use line comments instead of block comments.
            // can be load using single imm12.

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 created PR review comment:

Please use doc comments.
/// An imm20 immediate and a Imm12 immediate can generate what immediate.
/// imm20 + imm12
Also I think you should use standalone functions instead of a dummy type.

Wasmtime GitHub notifications bot (Jun 15 2022 at 11:42):

bjorn3 created PR review comment:

Also I think it should be called riscv64i or rv64i instead. This is the name of the base profile. It should be possible to enable and disable the extensions that form ricv64gc without requiring a completely new backend.

Wasmtime GitHub notifications bot (Jun 16 2022 at 01:05):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 16 2022 at 03:01):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 16 2022 at 05:15):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 16 2022 at 05:55):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 16 2022 at 13:07):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jun 16 2022 at 13:07):

bjorn3 created PR review comment:

        // calculate by test function riscv64_worst_case_size_instruction_size()

Wasmtime GitHub notifications bot (Jun 16 2022 at 22:10):

yuyang-ok created PR review comment:

ok.

Wasmtime GitHub notifications bot (Jun 16 2022 at 22:10):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jun 17 2022 at 22:47):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 18 2022 at 00:39):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 18 2022 at 06:25):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 23 2022 at 04:39):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 24 2022 at 01:43):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 24 2022 at 06:01):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 24 2022 at 07:12):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 26 2022 at 00:54):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 26 2022 at 01:46):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 26 2022 at 06:50):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 27 2022 at 01:09):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 27 2022 at 07:07):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 28 2022 at 01:34):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 28 2022 at 01:44):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 28 2022 at 02:37):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 28 2022 at 04:38):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 28 2022 at 05:07):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 28 2022 at 07:00):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jun 29 2022 at 04:58):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 02 2022 at 23:53):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 04 2022 at 07:05):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Why name just riscv64 and x64 here? Was this a copy+paste with find+replace on aarch64?

I think it's probably better to just say "we use the same generic ISA implementation as the other platforms", otherwise we have to update this comment in each platform when we add a new one :-)

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Can you say what ELF relocation kind this is equivalent to?

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

What does this comment mean, "all registers can be alloc"? ("allocated" maybe?) Does this mean all registers are potentially arguments? Please elaborate a bit more...

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

I'm not sure I understand this comment. Also, please don't have commented-out code.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Please don't use C-style block comments.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

this allocates a new Vec every time it's called; we don't need to do that, since there are only two possible vecs, one for Args and one for Rets. We should probably have a static array instead (let x_registers = &[...]).

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Please:

Don't use C-style block comments (/* ... */);

Fix the grammar here:

"when run out register , " --> "When we run out of registers, "

"we should use stack space for parameter" --> "We allocate remaining parameters on the stack."

"we should deal with paramter backwards" --> spelling ("parameter"), also what does this mean?

basically, I'm not sure I understand what this comment means, please clarify...

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Empty three-line block comment?

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Spaces between // start of comment and comment text

move out of doc-comment position and into function body

align the ;; assembly comments

s/sapce/space/, again (please spell-check comments in general or at least grep for this common typo!)

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

This code fragment comment should go inside the function, not in doc-comment position

The instruction add sp, sp-8 doesn't make sense to me; is this really how RISC-V writes it? Should it be add sp, sp, -8?

s/sapce/space/

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

This will need to be filled in.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Why was the unstable-sort commented out and replaced with sort here?

No commented-out code in comments, as mentioned multiple times above (please make a pass over the whole PR for this!)

If there was an issue with the original code, we should deal with it properly

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

s/Store/store/ (no need to capitalize)

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

fix grammar in comment?

"Returns whether a register must be saved by the callee" or something like that...

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

s/sources/source/

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Please replace this whole body of commented-out code with an implementation.

Re: temp register, is there an assembler temp or linker temp you can use? How would a call normally be codegen'd?

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

s/An/A/

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

What does this comment mean?

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

s/base on/based on/

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Space between ;; and comment.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

s/base on/based on/

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

CsrOp, not CsrOP, is generally the capitalization pattern we use for types here (Op is short for Operator which is one word, not an acronym).

Please use Lisp-style s-expr formatting:

Space between CsrOp and the (enum ...)

sub-forms (Csrrw), (Csrrs), ... indented under the (enum ...)

all closing parens on one line

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

formatting -- extra space

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Here and throughout all the ISLE files, please mind the extra blank lines: just one blank line between grouped definitions is enough.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

same formatting issue here as above

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

what does this comment mean?

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

paren on closing line should be at end of last form: ... rs2))

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

s/for emit/for emitting/

s/Interger/Integer/

(and likewise below)

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

FRM is too short; could we say FpRoundMode or similar? (And likewise for get_default_frm)

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Capitalize beginning of sentences: "Some ..."

s/instruction/instructions/

s/paramter/parameter/

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

formatting -- spaces around outside of parens. The standard way of writing this would be:

(decl alu_add (Reg Reg) Reg)

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Spaces around outside of parens

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

s/get negative/getting the negative/ (or just "negating")

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Indeed, this definitely shouldn't be in the backend-specific definitions. Also, isn't this already what def_inst does?

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

These sorts of converters should be in the general prelude, not in the RISC-V-specific one.

Also, implicit conversions between integer types are very dangerous, IMHO; I'd rather we be explicit about those conversions (as we are in Rust, with u32::try_from(x).unwrap() and such) to avoid accidental truncations or wraparounds or incorrect extensions...

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

s/for narrow down/for narrowing down/

s/it's/its/

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

empty comment?

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

opening paren should not be on a separate line (!) from its term: (alu_rr_imm12 ...) please!

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

This and the above can probably be replaced with lookup tables; otherwise the whole loop needs to be unrolled and optimized to produce code that is reasonable.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

This whole module will need to be filled in, and not be commented-out code.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

This test harness is actually pretty nice! However I think that we can't require a RISC-V toolchain to be installed in order to assemble the instructions. At best we could have an option to verify handwritten machine-code strings against instructions, if a certain Cargo feature is enabled. But the default tests should run without calling out to any other program like gcc.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Temporary code that should be removed.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

I don't think we really need to replicate the pattern from x64 where each individual register has its own function; areg(0), areg(1), is cleaner IMHO.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Also: if there is a standard document describing the calling convention for riscv64 (is it also called something like "SystemV" there?) could we link it here?

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

Is the commonly-accepted name as given for "platform" arguments just riscv64? Should the variants (G, C, IMAC`, others) be ISA flags instead?

Surveying e.g. Linux distros, I see that Fedora and Debian call the ISA riscv64; Rust uses riscv64gc in triples but riscv64 in a module name in the stdlib. In this PR the backend is in a riscv64 directory.

I'm fine either way I just want to make sure we give it a few seconds of thought :-)

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

+1 to has_m. Also, the descriptive text should actually say what the extension is, not just "extension M" -- that I could gather from the option name itself.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:10):

cfallin created PR review comment:

This should be implemented without dynamically allocating memory and building a Vec. Can you return a static &[Writable<Reg>] ?

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:29):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 01:45):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 02:24):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:03):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:03):

yuyang-ok created PR review comment:

not very clear, I found it in riscv-abi.pdf

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:05):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:05):

yuyang-ok created PR review comment:

riscv-abi.pdf

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:06):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:06):

yuyang-ok created PR review comment:

means potentially arguments.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:07):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:07):

yuyang-ok created PR review comment:

ok,I will try.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:08):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:08):

yuyang-ok created PR review comment:

look fine, I think this is better to read.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:08):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:08):

yuyang-ok created PR review comment:

ok.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:08):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:08):

yuyang-ok created PR review comment:

ok.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:22):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 04:22):

yuyang-ok created PR review comment:

yes I Copy it , I did not know where this information used.

Wasmtime GitHub notifications bot (Jul 07 2022 at 02:39):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 07 2022 at 05:41):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 07 2022 at 05:41):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 05:41):

yuyang-ok created PR review comment:

ok I have already remove it.

Wasmtime GitHub notifications bot (Jul 07 2022 at 05:56):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 07 2022 at 05:56):

yuyang-ok created PR review comment:

look I cannot because of

error[E0015]: cannot call non-const fn inst::regs::x_reg in constants

Wasmtime GitHub notifications bot (Jul 07 2022 at 06:03):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 05:35):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 08 2022 at 05:35):

yuyang-ok created PR review comment:

@cfallin some code are copied from aarch64. :)

Wasmtime GitHub notifications bot (Jul 08 2022 at 05:56):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 08 2022 at 05:56):

yuyang-ok created PR review comment:

@cfallin I aslo a little confused with platform name, riscv64gc is just my local test qemu system name.

Wasmtime GitHub notifications bot (Jul 08 2022 at 06:01):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 08 2022 at 08:18):

a1phyr submitted PR review.

Wasmtime GitHub notifications bot (Jul 08 2022 at 08:18):

a1phyr created PR review comment:

I think that x_reg could be made a const-fn, every function it calls is const or could be made const.

Wasmtime GitHub notifications bot (Jul 08 2022 at 11:26):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Jul 08 2022 at 11:26):

afonso360 created PR review comment:

Clang/LLVM have riscv64 but parse flags from the -march argument using the standard algorithm.

clang --target=riscv64-unknown-elf -march=rv64gcv

The above would translate to something like:
has_i
has_m
has_a
has_f
has_d
has_zicsr
has_zifencei
has_c
has_v
The RISCV Spec specifies how these flags should be parsed in Chapter 27 of the spec (page 149).

--

Having to specify all the flags individually might be tedious, but having gc enabled by default and enabling or disabling others using has_x I think might be a good compromise.

Or we could implement the parsing algorithm and require specifying some variation of rv64xxx along with the target.

Wasmtime GitHub notifications bot (Jul 08 2022 at 11:27):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 08 2022 at 11:28):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 08 2022 at 11:43):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Jul 11 2022 at 02:38):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 11 2022 at 08:03):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 13 2022 at 05:00):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 13 2022 at 06:41):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 14 2022 at 06:38):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 14 2022 at 06:41):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 14 2022 at 09:35):

bjorn3 created PR review comment:

Stack probing is a method to catch stack overflows and ensure the program aborts and not for example overwrites the heap. This is done by adding a guard page at the end of the stack for which all accesses cause a SIGSEGV. If stack frames are smaller than a page, nothing else needs to be done. If the stack frame is larger, a stack probe function is called which reads a single byte from every page in the stack frame to ensure that a stack overflow hits the guard page.

Wasmtime GitHub notifications bot (Jul 14 2022 at 09:35):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2022 at 01:41):

yuyang-ok created PR review comment:

ok,thanks.

Wasmtime GitHub notifications bot (Jul 16 2022 at 01:41):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2022 at 02:52):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2022 at 02:52):

yuyang-ok created PR review comment:

@a1phyr yes
![image](https://user-images.githubusercontent.com/96557710/179336523-3828ac41-1191-46b9-8ced-1af47c69fe2a.png)
but some function is not a const.

Wasmtime GitHub notifications bot (Jul 16 2022 at 03:41):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 16 2022 at 03:42):

yuyang-ok created PR review comment:

I have already remove the function "param_or_rets_xregs".

Wasmtime GitHub notifications bot (Jul 16 2022 at 03:42):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2022 at 03:45):

yuyang-ok edited PR #4271 from risc-v to main:

I am been trying to add riscv64 backend for cranelift these days.
right now I have pass all run test in filetests.

some features not implemented right now.
i128 mul div rem, all simd type and compare overflow.

some test need platform support.
like bitrev need qemu-riscv64 support bitmanip and zbkb extension (don't know how to enable it.).

Wasmtime GitHub notifications bot (Jul 16 2022 at 04:31):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Jul 16 2022 at 04:31):

yuyang-ok created PR review comment:

@bjorn3 I think I learnd this concept from an operating system course.

Wasmtime GitHub notifications bot (Jul 16 2022 at 04:31):

yuyang-ok deleted PR review comment.

Wasmtime GitHub notifications bot (Jul 16 2022 at 22:47):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Jul 25 2022 at 05:27):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 02 2022 at 05:38):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 07 2022 at 07:33):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 08 2022 at 00:35):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 08 2022 at 00:36):

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

Wasmtime GitHub notifications bot (Aug 08 2022 at 00:49):

yuyang-ok requested cfallin for a review on PR #4271.

Wasmtime GitHub notifications bot (Aug 08 2022 at 06:29):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 08 2022 at 07:16):

yuyang-ok created PR review comment:

I have change the name to riscv64.

Wasmtime GitHub notifications bot (Aug 08 2022 at 07:16):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 08 2022 at 08:21):

yuyang-ok deleted PR review comment.

Wasmtime GitHub notifications bot (Aug 08 2022 at 08:24):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 08 2022 at 08:24):

yuyang-ok created PR review comment:

Can you say what ELF relocation kind this is equivalent to?

@cfallin  I not quit understand the question.
I found this at riscv-abi.pdf .

![image](https://user-images.githubusercontent.com/96557710/183373482-e3cdf60e-1ff4-4fc8-b51d-93b151f9bbc3.png)

[riscv-abi.pdf](https://github.com/bytecodealliance/wasmtime/files/9280125/riscv-abi.pdf)

Wasmtime GitHub notifications bot (Aug 08 2022 at 08:30):

yuyang-ok created PR review comment:

Ok , thanks , I thinks I have remove all C-style block comments.

Wasmtime GitHub notifications bot (Aug 08 2022 at 08:30):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 08 2022 at 08:37):

yuyang-ok created PR review comment:

I have remove this comment.

Wasmtime GitHub notifications bot (Aug 08 2022 at 08:37):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 08 2022 at 08:38):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 08 2022 at 08:38):

yuyang-ok created PR review comment:

sorry about the typo.

Wasmtime GitHub notifications bot (Aug 08 2022 at 08:39):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 08 2022 at 08:39):

yuyang-ok created PR review comment:

removed. :-)

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:04):

yuyang-ok created PR review comment:

what should be the call convention for gen_probestack , I only found one call convention for risc-v on internet.

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:04):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:07):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:07):

yuyang-ok created PR review comment:

removed the comment. the code is copied from aarch64.

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:07):

yuyang-ok created PR review comment:

const fn should be ok??

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:07):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:24):

yuyang-ok deleted PR review comment.

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:42):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:42):

yuyang-ok created PR review comment:

ok. solved.

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:46):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:46):

yuyang-ok created PR review comment:

I want to remain this way.Is that ok?

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:58):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 08 2022 at 09:58):

yuyang-ok created PR review comment:

pub fn builder_with_options(infer_native_flags: bool) -> Result<isa::Builder, &'static str> {

I remove the infer_native_flags functionality for risc-v64 , I tried some time , but still compile not ok for stable tool chain.

Wasmtime GitHub notifications bot (Aug 10 2022 at 00:19):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 10 2022 at 05:19):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 11 2022 at 01:55):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 13 2022 at 01:35):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 16 2022 at 07:13):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 16 2022 at 09:41):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 16 2022 at 09:41):

afonso360 created PR review comment:

Usually SIMD types are in separate test files. Can you move the i8x16 test into a separate simd-popcnt.clif file and enable the scalar tests here?

Wasmtime GitHub notifications bot (Aug 16 2022 at 09:44):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 16 2022 at 09:44):

afonso360 created PR review comment:

Is this file deleted by accident?

Wasmtime GitHub notifications bot (Aug 16 2022 at 09:46):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 16 2022 at 09:46):

afonso360 created PR review comment:

It looks like we have lowerings for cls. Are these tests disabled for any particular reason?

Wasmtime GitHub notifications bot (Aug 16 2022 at 10:02):

afonso360 created PR review comment:

I think this might be an old comment, we now not only skip based on arch but check the native ISA flags to see if the host can run the particular test.

Wasmtime GitHub notifications bot (Aug 16 2022 at 10:02):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 16 2022 at 10:03):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 16 2022 at 10:08):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 17 2022 at 02:44):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 17 2022 at 04:25):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 04:25):

yuyang-ok created PR review comment:

Yes,I can. But I have not edit this file before.

Wasmtime GitHub notifications bot (Aug 17 2022 at 04:29):

yuyang-ok created PR review comment:

yes , I think so . I have recovered it.

Wasmtime GitHub notifications bot (Aug 17 2022 at 04:29):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 04:30):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 04:30):

yuyang-ok created PR review comment:

Yes, But My local qemu-riscv64 emulator doesn't implement it, I can't test it. right now I have two implemetion,
One rely on B extension , One don't.

Wasmtime GitHub notifications bot (Aug 17 2022 at 05:34):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 05:34):

yuyang-ok created PR review comment:

ok,I have merged the newest code.

Wasmtime GitHub notifications bot (Aug 17 2022 at 07:21):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 07:21):

afonso360 created PR review comment:

I think we don't need to duplicate all this code, we can probably do something along the lines of:

match (host_arch, requested_arch) {
  (host, requested) if host == requested => {},
  (Architecture::Riscv64(_), Architecture::Riscv64(_)) => {},
  _ => println!("skipped: ... etc""),
}

Wasmtime GitHub notifications bot (Aug 17 2022 at 07:23):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 07:23):

afonso360 created PR review comment:

Note, these are not duplicates of the ones below!

These targets, run this test file without the avoid_div_traps flag, and the ones below run the test file with the avoid_div_traps flag. Although we should probably clarify that in a comment

Wasmtime GitHub notifications bot (Aug 17 2022 at 08:08):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 18 2022 at 01:25):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 18 2022 at 02:24):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 18 2022 at 03:43):

yuyang-ok created PR review comment:

ok , thanks.

Wasmtime GitHub notifications bot (Aug 18 2022 at 03:43):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Aug 18 2022 at 07:19):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 23 2022 at 07:16):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 25 2022 at 02:45):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 26 2022 at 23:34):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 29 2022 at 07:57):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 31 2022 at 00:36):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Aug 31 2022 at 04:36):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 13 2022 at 19:53):

cfallin updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 13 2022 at 19:55):

cfallin updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 14 2022 at 00:58):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Sep 14 2022 at 00:58):

yuyang-ok created PR review comment:

ok, why I didn't see any test want to div by zero?

Wasmtime GitHub notifications bot (Sep 14 2022 at 00:59):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Sep 14 2022 at 00:59):

yuyang-ok created PR review comment:

what will cranelift expect to produce if 100/0 ??

Wasmtime GitHub notifications bot (Sep 14 2022 at 01:21):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 14 2022 at 01:21):

cfallin created PR review comment:

@yuyang-ok we get divide-by-zero coverage via the Wasm tests run by Wasmtime.

Wasmtime GitHub notifications bot (Sep 14 2022 at 04:39):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 15 2022 at 07:40):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 21 2022 at 00:17):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Sep 21 2022 at 00:17):

yuyang-ok created PR review comment:

@cfallin yes,But I think wasmtime test will all want to cause trap.
I see test in urem.clif set this flag.

function %urem_i64(i64, i64) -> i64 {
block0(v0: i64,v1: i64):
v2 = urem v0, v1
return v2
}
; run: %urem_i64(0, 1) == 0
; run: %urem_i64(2, 2) == 0
; run: %urem_i64(1, -1) == 1
; run: %urem_i64(3, 2) == 1
; run: %urem_i64(19, 7) == 5
; run: %urem_i64(3, -2) == 3
; run: %urem_i64(-19, 7) == 4
; run: %urem_i64(-57, -5) == -57
; run: %urem_i64(0, 104857600000) == 0
; run: %urem_i64(104857600000, 511) == 398
; run: %urem_i64(0xC0FFEEEE_DECAFFFF, 8) == 7
; run: %urem_i64(0xC0FFEEEE_DECAFFFF, -8) == 0xC0FFEEEE_DECAFFFF
; run: %urem_i64(0x80000000_00000000, -2) == 0x80000000_00000000

I didn't see you want to divide by zero.

Wasmtime GitHub notifications bot (Sep 21 2022 at 00:20):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 21 2022 at 00:23):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:10):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:10):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:10):

afonso360 created PR review comment:

It looks like this was accidentally included. I had to use this in my branch to test CI, but its probably something we don't want to include for everyone.

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:10):

afonso360 created PR review comment:

Yeah, We can't test traps via filetests yet! But it was something that was brought up recently in https://github.com/bytecodealliance/wasmtime/issues/4781.

Adding div by zero tests would be nice, but if we can't catch the traps properly we can't include it in the test suite (since it makes it crash)

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:10):

afonso360 created PR review comment:

These 3 lines test urem's without the avoid_div_traps enabled and are not duplicates, although that is perhaps not very clear.

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:10):

afonso360 deleted PR review comment.

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:12):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:12):

afonso360 created PR review comment:

Yeah, We can't test traps via filetests yet! But it was something that was brought up recently in #4781.

Adding div by zero tests would be nice, but if we can't catch the traps properly, we can't include it in the test suite (since it makes it crash).

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:17):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:19):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:43):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:43):

yuyang-ok created PR review comment:

look like we can test divide by zero in urem.clif because we can avoid_div_traps , but I didn't know whick value should produce.

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:43):

yuyang-ok edited PR review comment.

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:49):

yuyang-ok created PR review comment:

ok, I will recover it

Wasmtime GitHub notifications bot (Sep 21 2022 at 06:49):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Sep 22 2022 at 00:17):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Sep 22 2022 at 00:17):

yuyang-ok created PR review comment:

I didn't know why these 3 line show deleted.
![image](https://user-images.githubusercontent.com/96557710/191631869-f42b56ea-5baa-4265-b20d-f25a294424a0.png)

Wasmtime GitHub notifications bot (Sep 22 2022 at 06:49):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Sep 22 2022 at 06:49):

bjorn3 created PR review comment:

There were additional target lines above the ; Test these inputs without div traps, ... line that have been removed in this PR. Could you please add them back.

Wasmtime GitHub notifications bot (Sep 22 2022 at 22:32):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 22 2022 at 22:33):

yuyang-ok created PR review comment:

@bjorn3 sorry I didn't notice.

Wasmtime GitHub notifications bot (Sep 22 2022 at 22:33):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Sep 23 2022 at 01:57):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 23 2022 at 02:41):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 23 2022 at 04:59):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 23 2022 at 05:47):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 25 2022 at 01:53):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 26 2022 at 10:23):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Sep 26 2022 at 10:23):

afonso360 created PR review comment:

Are we using this feature flag anywhere? I can't see it.

Wasmtime GitHub notifications bot (Sep 26 2022 at 10:43):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Sep 26 2022 at 10:43):

yuyang-ok created PR review comment:

this is used.
![image](https://user-images.githubusercontent.com/96557710/192257511-76defd6e-5c74-4e38-b62b-af9b3e7141ec.png)

Wasmtime GitHub notifications bot (Sep 26 2022 at 10:47):

afonso360 created PR review comment:

That one seems to be the one on the cranelift-codegen crate, instead of the cranelift-native. For example we don't have the x86, arm64 or s390x features in this crate.

Wasmtime GitHub notifications bot (Sep 26 2022 at 10:47):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Sep 26 2022 at 10:50):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Sep 26 2022 at 11:14):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Sep 26 2022 at 11:14):

yuyang-ok created PR review comment:

@afonso360 sorry, I forget to remove it.

Wasmtime GitHub notifications bot (Sep 26 2022 at 11:14):

yuyang-ok created PR review comment:

deleted now.

Wasmtime GitHub notifications bot (Sep 26 2022 at 11:14):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Sep 27 2022 at 22:17):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 27 2022 at 22:17):

cfallin created PR review comment:

Why do we ignore the argument to the Riscv64 enum arm here? If we really need to special-case RISC-V in the toplevel test runner, can we factor the logic out somewhere else (to a "compatible RISC-V variant" helper function or something like that)?

Wasmtime GitHub notifications bot (Sep 27 2022 at 22:17):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 27 2022 at 23:46):

yuyang-ok submitted PR review.

Wasmtime GitHub notifications bot (Sep 27 2022 at 23:46):

yuyang-ok created PR review comment:

the current implementation is a little sloppy.

fn is_riscv64_compatible(

host: target_lexicon::Riscv64Architecture,
req: target_lexicon::Riscv64Architecture,
+) -> bool {
match host {
// Riscv64gc is short for RV64IMAFDCZicsr_Zifencei.
// So can run them all.
target_lexicon::Riscv64Architecture::Riscv64gc => true,
// Riscv64imac can run when req is not Riscv64gc.
target_lexicon::Riscv64Architecture::Riscv64imac
if req != target_lexicon::Riscv64Architecture::Riscv64gc =>
{
true
}
// Riscv64 is just basic extension.
target_lexicon::Riscv64Architecture::Riscv64
if req == target_lexicon::Riscv64Architecture::Riscv64 =>
{
true
}
_ => false,
}
+}
+
/// Checks if the host's ISA is compatible with the one requested by the test.
fn is_isa_compatible(
file_path: &str,
@@ -61,7 +85,8 @@ fn is_isa_compatible(

match (host_arch, requested_arch) {
(host, requested) if host == requested => {}
(Architecture::Riscv64(_), Architecture::Riscv64(_)) => {}
(Architecture::Riscv64(host), Architecture::Riscv64(req))
if is_riscv64_compatible(host, req) => {}
_ => {
return Err(format!(
"skipped {}: host can't run {:?} programs",

Wasmtime GitHub notifications bot (Sep 27 2022 at 23:47):

yuyang-ok updated PR #4271 from risc-v to main.

Wasmtime GitHub notifications bot (Sep 28 2022 at 00:29):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Sep 28 2022 at 00:30):

cfallin merged PR #4271.

Last updated: Nov 22 2024 at 17:03 UTC