Wasmtime GitHub notifications bot (Feb 01 2022 at 19:25): Wasmtime GitHub notifications bot (Feb 01 2022 at 19:25):fitzgen requested cfallin for a review on PR #3752.
Wasmtime GitHub notifications bot (Feb 01 2022 at 19:25):fitzgen opened PR #3752 from newtypes-for-register-classes to main:
This primary motivation of this large commit (apologies for its size!) is to
introduceGprandXmmnewtypes overReg. This should help catch
difficult-to-diagnose register class mixup bugs in x64 lowerings.But having a newtype for
GprandXmmthemselves isn't enough to catch all of
our operand-with-wrong-register-class bugs, because about 50% of operands on x64
aren't just a register, but a register or memory address or even an
immediate! So we have{Gpr,Xmm}Mem[Imm]newtypes as well.Unfortunately,
GprMemet al can't beenums and are therefore a little bit
noisier to work with from ISLE. They need to maintain the invariant that their
registers really are of the claimed register class, so they need to encapsulate
the inner data. If they exposed the underlyingenumvariants, then anyone
could just change register classes or construct aGprMemthat holds an XMM
register, defeating the whole point of these newtypes. So when working with
these newtypes from ISLE, we rely on external constructors like(gpr_to_gpr_mem my_gpr)instead of(GprMem.Gpr my_gpr).A bit of extra lines of code are included to add support for register mapping
for all of these newtypes as well. Ultimately this is all a bit wordier than I'd
hoped it would be when I first started authoring this commit, but I think it is
all worth it nonetheless!In the process of adding these newtypes, I didn't want to have to update both
the ISLEexterntype definition ofMInstand the Rust definition, so I move
the definition fully into ISLE, similar as aarch64.Finally, this process isn't complete. I've introduced the newtypes here, and
I've made most XMM-using instructions switch fromRegtoXmm, as well as
register class-converting instructions, but I haven't moved all of the GPR-using
instructions over to the newtypes yet. I figured this commit was big enough as
it was, and I can continue the adoption of these newtypes in follow up commits.Part of #3685.
Wasmtime GitHub notifications bot (Feb 03 2022 at 00:18):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Feb 03 2022 at 00:18):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Feb 03 2022 at 00:18):cfallin created PR review comment:
Would these constructors be clearer without the
_newsuffix, I wonder? I'm mostly thinking in terms of symmetry with e.g. instruction constructors and the other utility types -- we have(value_regs ...), not(value_regs_new ...). (Or maybe there's a name conflict from elsewhere I'm missing?)
Wasmtime GitHub notifications bot (Feb 03 2022 at 19:18):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Feb 03 2022 at 19:18):fitzgen created PR review comment:
I can do
reg_mem_to_xmm_memwhich is more similar to the other constructors. Thoughts?
Wasmtime GitHub notifications bot (Feb 03 2022 at 19:24):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Feb 03 2022 at 19:24):cfallin created PR review comment:
That seems reasonable to me! I guess as a general principle the
x_to_yexplicitness is good and is a sort of direct indication that we can register all such constructors as implicit conversions later.
Wasmtime GitHub notifications bot (Feb 03 2022 at 19:34):fitzgen updated PR #3752 from newtypes-for-register-classes to main.
Wasmtime GitHub notifications bot (Feb 03 2022 at 19:46):fitzgen updated PR #3752 from newtypes-for-register-classes to main.
Wasmtime GitHub notifications bot (Feb 03 2022 at 20:33):fitzgen updated PR #3752 from newtypes-for-register-classes to main.
Wasmtime GitHub notifications bot (Feb 03 2022 at 22:08):fitzgen updated PR #3752 from newtypes-for-register-classes to main.
Wasmtime GitHub notifications bot (Feb 03 2022 at 22:56):fitzgen merged PR #3752.
Last updated: Dec 23 2024 at 12:05 UTC