Stream: git-wasmtime

Topic: wasmtime / Issue #1665 Add initial scalar FP operations (...

view this post on Zulip Wasmtime GitHub notifications bot (May 20 2020 at 20:04):

jlb6740 commented on Issue #1665:

@julian-seward1 @cfallin Rebased for Chris's latest updates.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2020 at 23:36):

jlb6740 commented on Issue #1665:

@julian-seward1 Thanks for the suggestions! I like somehow capturing the CPUID feature (SSE, SSE2, AVX, etc) and wanted to do this in an earlier push but wasn't sure how we'd want it done. It's good to encode this information now so good suggestion. On that note, I was thinking SSE_OP should really be SSE_RM_OP for consistency and convenient cross checking purposes. We'll want SSE_RMI_OP (actually created here already), SSE_RVM_OP, etc in the future. Do you think this would be fine to add (maybe update in a future patch)?

Also potentially in a future patch I am not sure the value of having XMM_R_R. Seems this instruction format can be capture with XMM_RM_M ... same reason we combined SSE_RM_M and SSE_R_R. It would simplify logic in a few places. Do you think we can remove this as well in another patch?

Note, one thing I didn't do here in this patch was remove the F_PREFIX_66 since that is a valid prefix that I think we will need. If you still think its best to remove I can push the removal.

Also separately I noticed that it doesn't appear that cargo run -- wasm disassemble is printing based on lowered vcode. I say that because I redundant rex bits appear to be retained in the binary when I don't expect that. I am thinking this is printing from original cranelift lowering? Will have to investigate this further but will probably open this as an issue and submit a patch.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 00:54):

jlb6740 edited a comment on Issue #1665:

@julian-seward1 Thanks for the suggestions! I like somehow capturing the CPUID feature (SSE, SSE2, AVX, etc) and wanted to do this in an earlier push but wasn't sure how we'd want it done. It's good to encode this information now so good suggestion. On that note, I was thinking SSE_OP should really be SSE_RM_OP for consistency and convenient cross checking purposes. We'll want SSE_RMI_OP (actually created here already), SSE_RVM_OP, etc in the future. Do you think this would be fine to add (maybe update in a future patch)?

Also potentially in a future patch I am not sure the value of having XMM_R_R. Seems this instruction format can be capture with XMM_RM_M ... same reason we combined SSE_RM_M and SSE_R_R. It would simplify logic in a few places. Do you think we can remove this as well in another patch?

Note, one thing I didn't do here in this patch was remove the F_PREFIX_66 since that is a valid prefix that I think we will need. If you still think its best to remove I can push the removal.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 07 2020 at 07:35):

julian-seward1 commented on Issue #1665:

Ah, you're right, Inst::XMM_R_R is redundant because Inst::XMM_RM_R will cover that case. Not sure what I was thinking there.

The reason I proposed simply SSE_Op and not SSE_RM_R_Op and SSE_R_R_Op etc is that for almost all SSE(*) insns, both the reg-mem and reg-reg forms exist. Hence if we did that we'd get a lot of duplication in the two enums.

Regarding SSE_RMI_OP op, there are no SSE insns where one of the operands is a literal (immediate) value, so this would never get used. There are a few SSE insns in which there is an immediate 8-bit value, but that is basically an opcode extension, not an operand, and they are rare, so we can special-case those later if we need them.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 10 2020 at 16:36):

bnjbvr commented on Issue #1665:

Merging, so we can move forward. Thanks!

Last updated: Dec 23 2024 at 12:05 UTC