Wasmtime GitHub notifications bot (Jun 11 2020 at 15:02): Wasmtime GitHub notifications bot (Jun 11 2020 at 15:02):bnjbvr opened PR #1863 from x64 to master:
This renames and encapsulates some concepts a bit more; no functional changes have been done, as confirmed by the tests still passing :-)
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:02):bnjbvr requested julian-seward1 for a review on PR #1863.
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:49):bnjbvr updated PR #1863 from x64 to master:
This renames and encapsulates some concepts a bit more; no functional changes have been done, as confirmed by the tests still passing :-)
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:55):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:55):julian-seward1 created PR Review Comment:
I see from reading later that we can extract the SSE level by passing calling its
.available_frommethod. But I don't see the win here .. it places more of a burden on readers of the code, who now have to look in some other place to figure out the SSE level.
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:55):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:55):julian-seward1 created PR Review Comment:
Ahh ... this renaming loses the SSE-level on the name; I specifically wanted to retain that -- per discussion with Johnny in PR1665. Can you reinstate it somehow? I think it will be confusing and difficult -- especially later, when we add even more opcodes (and there are zillions of them) -- if we cannot look at an SseOpcode value and know directly what level it is for.
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:55):julian-seward1 created PR Review Comment:
Hmm .. I'm not entirely happy about the
iinRmihere. Those immediates are often an opcode extension, especially for the comparison instructions, and not really an operand. By contrast, theiinAluRmireally is an operand. Hence there's a conceptual inconsistency. Not sure what a better name is though.
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:55):julian-seward1 created PR Review Comment:
At some point this will need to be expanded to include whole-register copies (mova/movu).
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:55):julian-seward1 created PR Review Comment:
What is this used for? I don't see a previous version of it, and I feel nervous about having a function that gets the encoding for a register without checking the class. (cf inconsistency with
int_reg_enc). At some point soon I guess we'll need anxmm_reg_encas a dual toint_reg_enc?
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:55):julian-seward1 created PR Review Comment:
I'm not sure what the sentence "A select prefix .. " means (what's a "select" prefix?) Maybe rm this sentence? In any case none of this machinery will work for creating VEX- or EVEX-prefixed instructions; we'll have to redo it for those.
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:55):julian-seward1 created PR Review Comment:
Hmm, also here is inconsistent now.
There are two axes of differentiation in the old naming:
- E is reg vs E is mem (G is always a reg)
- whether reg values are indicated by passing a Reg ("reg") or just its encoding number ("enc")
Can you rename so as to bring back those distinctions?
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:55):julian-seward1 created PR Review Comment:
Can this be called
RexFlags? Calling it simplyRexmight lead one to believe that it itself is a Rex prefix, when it is not.
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:55):julian-seward1 created PR Review Comment:
Hmm, this renaming hides the fact that the E operand is memory.
.._enc_g_mem_e?
Wasmtime GitHub notifications bot (Jun 11 2020 at 15:58):julian-seward1 edited PR Review Comment.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:09):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:09):bnjbvr created PR Review Comment:
Sorry, i should have commented a bit in the commit message. With the
available_fromfunction, we can then automate the checks that the instructions are available when comparing to a given SSE level. So this could be used to dynamically (at runtime) check that we only use allowed instructions, as well as selecting instructions based on the real SSE level.So it's slightly more indirection, but with this check (to come in a subsequent pull request), there's no way it can be misused. In terms of developer experience, in most modern editors, this is a "jump to definition" then grep for "available_from", which is not too bad (and should not be done so often). Overall I found better to have something that could be probed and checked programmatically rather than a "visual" check that's subject to operators errors :)
With this new information, do you agree with the current form, or do you strongly feel about reverting it?
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:11):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:11):bnjbvr created PR Review Comment:
I see. What about
SseRmIOpcode(with capital I), to indicate it is its own operand? Or in a more verbose way,SseRmImmOpcode?
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:12):bnjbvr created PR Review Comment:
There were two TODOs (removed in this patch) that said that two
int_reg_enccalls should really be replaced with a newreg_enc, because registers that could flow in could be either GPRs or SSE. Hence this new function.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:12):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:12):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:12):bnjbvr created PR Review Comment:
This is preexisting :-) Maybe "a given prefix" would make more sense here?
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:12):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:12):bnjbvr created PR Review Comment:
Sure!
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:14):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:14):bnjbvr created PR Review Comment:
Hmm, my memories of x86 told me that SIB could only be used with a memory operand, so the presence of "sib" in the name implied a memory operand. Besides, I was trying to shorten the name a bit :-) But yeah, I could add it back and it wouldn't be too terrible.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:18):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:18):bnjbvr created PR Review Comment:
I thought I kept this distinction with the "enc" that's before "ge", and "reg" before "ge" in the other variants, but I see that I used "rm" for the Reg/Addr variant of
emit_modrm_sib_enc_ge. Let's discuss it offline.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:54):bnjbvr updated PR #1863 from x64 to master:
This renames and encapsulates some concepts a bit more; no functional changes have been done, as confirmed by the tests still passing :-)
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:54):bnjbvr requested julian-seward1 for a review on PR #1863.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:55):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 11 2020 at 16:55):bnjbvr created PR Review Comment:
We settle on
emit_std_{enc,reg}_{mem,reg}variants here.
Wasmtime GitHub notifications bot (Jun 12 2020 at 17:09):bnjbvr updated PR #1863 from x64 to master:
This renames and encapsulates some concepts a bit more; no functional changes have been done, as confirmed by the tests still passing :-)
Wasmtime GitHub notifications bot (Jun 12 2020 at 17:26):bnjbvr updated PR #1863 from x64 to master:
This renames and encapsulates some concepts a bit more; no functional changes have been done, as confirmed by the tests still passing :-)
Wasmtime GitHub notifications bot (Jun 15 2020 at 09:17):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jun 15 2020 at 09:17):julian-seward1 created PR Review Comment:
That still doesn't make clear that the imm isn't an operand. How about
SseRmExtendedOpcode?
Wasmtime GitHub notifications bot (Jun 15 2020 at 09:17):julian-seward1 edited PR Review Comment.
Wasmtime GitHub notifications bot (Jun 15 2020 at 09:45):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jun 15 2020 at 09:45):julian-seward1 submitted PR Review.
Wasmtime GitHub notifications bot (Jun 15 2020 at 09:45):julian-seward1 created PR Review Comment:
Actually .. thinking about this more .. I don't think there is any point in having these be separate from the plain
SseOpcode. They are "just more" SSE* instructions and the only difference is that the encoding of the instruction has an extra byte, which IIRC is always right at the end of the instruction. So I think you should merge these intoSseOpcodeand deleteSseRmiOpcode. (w/ apologies for changing my story on this)
Wasmtime GitHub notifications bot (Jun 15 2020 at 13:52):bnjbvr updated PR #1863 from x64 to master:
This renames and encapsulates some concepts a bit more; no functional changes have been done, as confirmed by the tests still passing :-)
Wasmtime GitHub notifications bot (Jun 15 2020 at 13:52):bnjbvr submitted PR Review.
Wasmtime GitHub notifications bot (Jun 15 2020 at 13:52):bnjbvr created PR Review Comment:
Discussed on irc and Julian agreed with this, resolving.
Wasmtime GitHub notifications bot (Jun 15 2020 at 14:00):bnjbvr updated PR #1863 from x64 to master:
This renames and encapsulates some concepts a bit more; no functional changes have been done, as confirmed by the tests still passing :-)
Wasmtime GitHub notifications bot (Jun 15 2020 at 14:39):bnjbvr merged PR #1863.
Last updated: Dec 23 2024 at 13:07 UTC