wasmtime / PR #1665 In progress. · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #1665 In progress.

Wasmtime GitHub notifications bot (May 06 2020 at 07:59):

jlb6740 opened PR #1665 from vcode-add-movss-addss-subss to master:

Need to better separate data structures for sse and regular regs

Lowering is not right .. made sure all paths towards lowering are
covered but still need to review what is there and correct

Need to emit code. Currently have placeholders.

Wasmtime GitHub notifications bot (May 06 2020 at 08:49):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2020 at 08:49):

bjorn3 created PR Review Comment:

/// These indicate ways of extending (widening) a value, using the Intel
/// naming: B(yte) = u8, W(ord) = u16, L(ong)word = u32, Q(uad)word = u64
#[derive(Clone, PartialEq)]

Wasmtime GitHub notifications bot (May 06 2020 at 08:50):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2020 at 08:50):

bjorn3 created PR Review Comment:

/// Some basic ALU operations.
#[derive(Clone, PartialEq)]

Wasmtime GitHub notifications bot (May 06 2020 at 08:50):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2020 at 08:50):

bjorn3 created PR Review Comment:

pub(crate) fn xmm0() -> Reg {

Wasmtime GitHub notifications bot (May 06 2020 at 08:52):

bjorn3 created PR Review Comment:

Probably just forgotten.

Wasmtime GitHub notifications bot (May 06 2020 at 08:52):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2020 at 08:54):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2020 at 08:54):

bjorn3 created PR Review Comment:

Otherwise self.vcode.vreg_types[vreg.get_index()] would panic on out of bounds indexing.

Wasmtime GitHub notifications bot (May 06 2020 at 09:30):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2020 at 09:30):

julian-seward1 created PR Review Comment:

We should really rename both this and the existing RM_R_Op to something more sensible. For RM_R_Op I'm not sure what. For this, maybe SSE2_BinOp would be good. Basically all two-operand SSE2 instructions (the R/R and R/M) versions can be represented this way, so it's nice and compact.

Wasmtime GitHub notifications bot (May 06 2020 at 09:49):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2020 at 09:49):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2020 at 09:49):

julian-seward1 created PR Review Comment:

Errr, this is tricky, for reasons to do with copy coalescing in regalloc. At least for now, I'd prefer to have an Xmm-to-Xmm move instruction that operates only on the entire register. We can come back and refine this later. So you'd drop the Scalar bit here, giving SSE_Mov_R_R. And correspondingly emit whatever Intel recommends for Xmm-to-Xmm moves .. is it movapd reg, reg perhaps?

Wasmtime GitHub notifications bot (May 06 2020 at 09:49):

julian-seward1 created PR Review Comment:

Per comments above, this would want to be SSE_BinOp_RM_R.

Wasmtime GitHub notifications bot (May 06 2020 at 09:49):

julian-seward1 created PR Review Comment:

I guess RM_R_Op should then be changed to INT_RM_R_Op.

Wasmtime GitHub notifications bot (May 06 2020 at 09:49):

julian-seward1 created PR Review Comment:

Hence, sse_mov_r_r

Wasmtime GitHub notifications bot (May 06 2020 at 09:49):

julian-seward1 created PR Review Comment:

Not 100% sure I understand the question, but .. let's keep the XMM and integer insns separate. So I think the answer is then "no".

Wasmtime GitHub notifications bot (May 06 2020 at 09:49):

julian-seward1 created PR Review Comment:

Because they have never actually been used so far.

Wasmtime GitHub notifications bot (May 06 2020 at 09:49):

julian-seward1 created PR Review Comment:

Per comments above, I'd prefer that mov_r_r continued to be for int-only moves, but renamed to mov_int_r_r, and we make a new one that's only for xmm regs, called mov_xmm_r_r.

Wasmtime GitHub notifications bot (May 06 2020 at 09:49):

julian-seward1 created PR Review Comment:

I'd prefer you called this ty_is_flt64 and rename its integer counterpart to ty_is_int64.

Wasmtime GitHub notifications bot (May 06 2020 at 10:03):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (May 06 2020 at 10:03):

julian-seward1 created PR Review Comment:

This is old-x86-backend-world, btw.

Wasmtime GitHub notifications bot (May 07 2020 at 07:07):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 07:07):

jlb6740 created PR Review Comment:

Yes I see ... initially I wasn't sure why the need for the loop. I don't see a grow() for vectors but perhaps instead of looping we can replace with a resize statement? Something like:

self.vcode.vreg_types.resize(ir::types::I8, vreg.get_index() - self.vcode.vreg_types.len())

Wasmtime GitHub notifications bot (May 07 2020 at 07:09):

jlb6740 edited PR Review Comment.

Wasmtime GitHub notifications bot (May 07 2020 at 07:36):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 07:36):

jlb6740 created PR Review Comment:

What prompted this comment was here in lower.rs:

https://github.com/bytecodealliance/wasmtime/blob/48521393ae6b6e014902fac8fe70ac2c1b221880/cranelift/codegen/src/isa/x64/lower.rs#L187

For a lot of opcodes we can specify from here which Inst::mov variant we want to call but for return for example, multiple types apply and I found I was going to only be calling one version which might not be the correct one. I was going to check types in lower.rs, but instead I created a mov specific to the return op and put the decision logic there.

Wasmtime GitHub notifications bot (May 07 2020 at 09:01):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 09:01):

bjorn3 created PR Review Comment:

vreg.get_index() may be smaller than self.vcode.vreg_types.len(), in which case the substraction will underflow.

Wasmtime GitHub notifications bot (May 07 2020 at 09:30):

julian-seward1 submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 09:30):

julian-seward1 created PR Review Comment:

Ah, that is a good point. Yes. True. Nevertheless, the general flavour of the rest of the instruction selector (lower.rs) is that we keep int and vec instructions separate, and we don't hand that problem to the instruction-representation layer. So the lowering logic to handle Opcode::FallthroughReturn | Opcode::Return will need to do the type test itself, rather than handing it off to return_mov_r_r. FWIW I think there are only very few places like this, where we need to check int-vs-vec/float types at the lowering level -- in prologue construction is the only other place I can think of.

Wasmtime GitHub notifications bot (May 07 2020 at 16:21):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 16:21):

jlb6740 created PR Review Comment:

I kept the guard. It's just local, but it looks like this:
    if self.vcode.vreg_types.len() < vreg.get_index() {
        self.vcode.vreg_types.resize(
            vreg.get_index() - self.vcode.vreg_types.len(),
            ir::types::I8,
        )
    }
    self.vcode.vreg_types[vreg.get_index()] = ty;
as opposed to this with the while loop:
    while self.vcode.vreg_types.len() <= vreg.get_index() {
        self.vcode.vreg_types.push(ir::types::I8); // Default type.
    }
    self.vcode.vreg_types[vreg.get_index()] = ty;
I assumed that top "if-branch would be more efficient but not sure what rust is doing under the covers for the resize compared to the push.

Wasmtime GitHub notifications bot (May 07 2020 at 16:55):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 16:55):

bnjbvr created PR Review Comment:

In general, a resize may reallocate all the memory you need at once, while several pushes in a row may cause several series of reallocation, so resize is to be preferred. (I don't know if rustc even gets to use some kind of SIMD tricks to fill the memory with the default value.)

Wasmtime GitHub notifications bot (May 07 2020 at 17:29):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 17:29):

jlb6740 created PR Review Comment:

@Bnjbvr ... Cool thanks. Good to know.

Wasmtime GitHub notifications bot (May 07 2020 at 17:29):

jlb6740 deleted PR Review Comment.

Wasmtime GitHub notifications bot (May 07 2020 at 17:30):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 17:30):

jlb6740 created PR Review Comment:

@bnjbvr .. Cool thanks.

Wasmtime GitHub notifications bot (May 07 2020 at 17:30):

jlb6740 edited PR Review Comment.

Wasmtime GitHub notifications bot (May 07 2020 at 17:31):

jlb6740 deleted PR Review Comment.

Wasmtime GitHub notifications bot (May 07 2020 at 17:33):

jlb6740 submitted PR Review.

Wasmtime GitHub notifications bot (May 07 2020 at 17:33):

jlb6740 created PR Review Comment:

@bnjbvr Ok thanks. That SIMD question is an interesting one.

Wasmtime GitHub notifications bot (May 07 2020 at 18:12):

jlb6740 edited PR Review Comment.