Stream: git-wasmtime

Topic: wasmtime / PR #2004 machinst x64: floating point support


view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 15:52):

bnjbvr edited PR #2004 from x64-fp to main:

Opening this PR as a WIP, to coordinate better with @jlb6740; this lacks proper testing right now. Only the last commit is relevant, the rest comes from #1995.

This implements the following:

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 16:17):

bnjbvr updated PR #2004 from x64-fp to main:

Opening this PR as a WIP, to coordinate better with @jlb6740; this lacks proper testing right now. Only the last commit is relevant, the rest comes from #1995.

This implements the following:

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 16:25):

bnjbvr updated PR #2004 from x64-fp to main:

Opening this PR as a WIP, to coordinate better with @jlb6740; this lacks proper testing right now. Only the last commit is relevant, the rest comes from #1995.

This implements the following:

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

What does this function do / how does it help? I can't guess from the name, and reading the code doesn't help much. Can you add a comment? Maybe rename it?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

I look at this (the introduction of is_int) and imagine the day when this needs to be extended again to handle 128-bit vector registers. That day can't be far off. In that case even is_int will be inadequate, I guess. Would it be more future-proof to return a RegClass instead of the bool here?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

Hmm, this comment isn't really right. Is XMM_RM_R only used for scalar floating point? I would have thought it could be (and, eventually, will be) used for any SSE operation of the form R := OP(R, RM).

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

nit: XmmUnary --> XmmUnaryRmR

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

nit: "from float to reg/mem" isn't right. These are stores (they can't be anything else), and AFAIK they could also be stores of the entire vector register, not just the lowest F32 or F64 lane.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

As a suggestion, would you consider renaming xmm_to_int to xmm_to_gpr (and the associated data structures?) The reason is that "xmm" is unambiguously an XMM register. But "int" isn't so clear; it could mean "integer register" (as you mean here) but it could also be interpreted as "integer value stored in some lane(s) of an XMM register". _to_gpr removes that ambiguity.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

What happens to the upper 96/64 bits of the destination? I don't think it is important for code generation, but it would be nice if what happens was stated here.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

Just as a comment .. these assertions are good (I like assertions), but .. won't the emit function eventually assert/panic if the wrong class register is in an instruction field? I think that's true but I don't remember exactly. So I think you're safe even without the assert. (and if the emit fns don't assert/panic in such circumstances, we should fix them so they do; that's the last line of defense against mistakes in register classes).

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

On reading further down, I see this appears to be used to assert the register class. Would you consider renaming this to assert_regclass_is? I'd prefer to avoid the term "register type" since that's not really a concept we have; what we have are the concepts "register class" and "type of the value stored in the register", which may not be the same. Eg a F32 value in a V128-class register.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

isn't it the inverse-of-cc that needs to be printed here?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

We can probably special-case 0.0 using xorpd or whatever the official dependency-breaking generate-vector-zero insn is. But not now; we can come back to this.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

Whatever is chosen, make sure it's an officially-recognised dependency-breaking insn.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

Can this ever fail for valid input?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:26):

julian-seward1 created PR Review Comment:

Oh, you comment that just below :-)

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:27):

julian-seward1 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 09:27):

julian-seward1 created PR Review Comment:

Oh, maybe that's a stupid idea, because F64 and F32s live in V128 class registers, and we need a way to distinguish between these three cases. So ignore this.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 12:46):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 12:46):

bnjbvr created PR Review Comment:

Indeed, done! I've also removed the use of a match to just use if let ... direct pattern matching, which reduces code indent by one tab.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 12:58):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 12:58):

bnjbvr created PR Review Comment:

I renamed it XmmCmove for consistency, and precised that it applied only to scalars at this point; or we can just generalize it right now and use movapd consistently here, not sure...

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 13:01):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 13:01):

bnjbvr created PR Review Comment:

The emit functions don't protect much against this, at this point. You're right it'd be better if they did, i'll make this in a subsequent PR!

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 13:02):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 13:02):

bnjbvr created PR Review Comment:

good catch, thx

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 13:05):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 13:05):

bnjbvr created PR Review Comment:

In theory, yes: the complex variants can take as many inputs as the user wants, and the documentation says it'll add all the inputs together to form the effective address. In practice, postopt I think is the only creator of these instruction variants, and will only use up to 3 inputs together, to map to what the x86 architecture can do.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 13:12):

bnjbvr updated PR #2004 from x64-fp to main:

Opening this PR as a WIP, to coordinate better with @jlb6740; this lacks proper testing right now. Only the last commit is relevant, the rest comes from #1995.

This implements the following:

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 13:56):

bnjbvr merged PR #2004.


Last updated: Dec 23 2024 at 12:05 UTC