Wasmtime GitHub notifications bot (May 18 2023 at 09:15): Wasmtime GitHub notifications bot (May 18 2023 at 09:15):remlse edited PR #6407:
related to #4134
Wasmtime GitHub notifications bot (May 18 2023 at 11:30):remlse updated PR #6407.
Wasmtime GitHub notifications bot (May 18 2023 at 18:24):cfallin submitted PR review:
This looks basically like what I expected -- I think it's a good addition to the suite of chaos techniques!
Wasmtime GitHub notifications bot (May 18 2023 at 18:24):cfallin submitted PR review:
This looks basically like what I expected -- I think it's a good addition to the suite of chaos techniques!
Wasmtime GitHub notifications bot (May 18 2023 at 18:24):cfallin created PR review comment:
We could perhaps choose a float or int here, so we get the increased register pressure / potential clobbering of both kinds of registers. I guess that implies two kinds of
gen_immas well.
Wasmtime GitHub notifications bot (May 22 2023 at 21:40):remlse updated PR #6407.
Wasmtime GitHub notifications bot (May 22 2023 at 21:42):remlse created PR review comment:
The floats aren't working yet, I still have to figure out how to properly use a floating point register.
Wasmtime GitHub notifications bot (May 25 2023 at 09:43):remlse updated PR #6407.
Wasmtime GitHub notifications bot (May 25 2023 at 10:02):remlse created PR review comment:
An immediate floating point value seems to compile to the immediate value being stored in a general purpose register first, then moved to a floating point register (xmm).
So I did it the same way here. But I assume the abstraction is not right yet. The
MachInsttrait shouldn't contain references to implementation details of a specific backend.If the
MachInsttrait had some methodgen_imm_f64, what should it return? Just one of the existing instructions is not enough, I think, because a single instruction for immediate f64 values doesn't exist? And returning something likeOption<(Self, Self)>also seems like a leaky abstraction.@cfallin maybe you have an idea for the right abstraction here?
Wasmtime GitHub notifications bot (May 25 2023 at 18:11):cfallin created PR review comment:
@remlse a pattern we've used before successfully (see e.g. some instances in
abi.rs) is to return aSmallVec<[Inst; 2]>(tweak inlined size as appropriate for expected max size). Then we have something likefor inst in M::gen_seq(...) { self.emit(inst); }or whatever is appropriate at the callsite.
Wasmtime GitHub notifications bot (May 26 2023 at 07:39):remlse updated PR #6407.
Wasmtime GitHub notifications bot (May 26 2023 at 07:55):remlse created PR review comment:
That works well, thanks.
MachInst::gen_imm_f64still "leaks" the fact that there may be a temporary general purpose register required, are you ok with that?
Wasmtime GitHub notifications bot (May 26 2023 at 18:41):cfallin created PR review comment:
Yep, that's fine, we have a notion of "this machine-specific sequence gets a tmp" elsewhere too. If you're happy with this please feel free to flip it to Ready to Review and I'll look over the whole thing!
Wasmtime GitHub notifications bot (May 26 2023 at 20:05):remlse has marked PR #6407 as ready for review.
Wasmtime GitHub notifications bot (May 26 2023 at 20:05):remlse requested elliottt for a review on PR #6407.
Wasmtime GitHub notifications bot (May 26 2023 at 20:05):remlse requested wasmtime-compiler-reviewers for a review on PR #6407.
Wasmtime GitHub notifications bot (Jun 01 2023 at 12:43):remlse created PR review comment:
@cfallin pinging in case you weren't notified when I marked the PR ready for review
Wasmtime GitHub notifications bot (Jun 01 2023 at 16:03):cfallin created PR review comment:
Ah, thanks, indeed that does not send an email so I missed it. Taking a final look now.
Wasmtime GitHub notifications bot (Jun 01 2023 at 16:04):cfallin submitted PR review:
LGTM!
Wasmtime GitHub notifications bot (Jun 01 2023 at 16:43):cfallin merged PR #6407.
Last updated: Dec 23 2024 at 13:07 UTC