Stream: git-wasmtime

Topic: wasmtime / PR #4389 x64: port `atomic_rmw` to ISLE

Wasmtime GitHub notifications bot (Jul 05 2022 at 23:47):

abrown opened PR #4389 from isle-atomic-rmw to main:

This change ports atomic_rmw to ISLE for the x64 backend. It does not
change the lowering in any way, though it seems possible that the fixed
regs need not be as fixed and that there are opportunities for single
instruction lowerings. It does rename inst_common::AtomicRmwOp to
MachAtomicRmwOp to disambiguate with the IR enum with the same name.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jul 06 2022 at 00:48):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 00:48):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 00:48):

cfallin created PR review comment:

Let's keep some variant of this FIXME around: it would still be ideal to not have hardcoded registers in the sequence, but rather allocate as many temps as needed (as operands) and use them here.

Wasmtime GitHub notifications bot (Jul 06 2022 at 00:48):

cfallin created PR review comment:

No indent here

Wasmtime GitHub notifications bot (Jul 06 2022 at 00:48):

cfallin created PR review comment:

Can we assert something about flags? (We could pass it into x64_atomic_rmw_seq and match only the flags that we expect, or something like that...)

Wasmtime GitHub notifications bot (Jul 06 2022 at 00:48):

cfallin created PR review comment:

These two should be "late uses", or otherwise the below two defs should be "early defs", semantically. It won't matter yet with the fixed reg constraints (neither of the defs could possibly overlap either of the uses) but it will matter when we eventually remove the need for the fixed regs, and I'd rather note it now while we're thinking about it!

I don't think there is a reg_fixed_late_use on the OperandCollector but we should be able to add one, analogously to the other helpers there.

Wasmtime GitHub notifications bot (Jul 06 2022 at 00:48):

cfallin created PR review comment:

Likewise here, let's have some sort of TODO noting that ideally we wouldn't have register constraints.

Wasmtime GitHub notifications bot (Jul 06 2022 at 02:40):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 02:40):

abrown created PR review comment:

Ok, I might actually add another commit to this PR to avoid the hard coded registers... Just wanted to make sure this was on the right track. (Or I could open another PR, I guess).

Wasmtime GitHub notifications bot (Jul 06 2022 at 15:37):

abrown updated PR #4389 from isle-atomic-rmw to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 15:46):

abrown updated PR #4389 from isle-atomic-rmw to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 15:46):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 15:46):

abrown created PR review comment:

I think we probably want a fixed_reg_use_at_end helper in regalloc2. See what I did in OperandCollector in the interim.

Wasmtime GitHub notifications bot (Jul 06 2022 at 15:47):

abrown created PR review comment:

Not sure what we should assert here.

Wasmtime GitHub notifications bot (Jul 06 2022 at 15:47):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 15:56):

abrown updated PR #4389 from isle-atomic-rmw to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 18:11):

abrown updated PR #4389 from isle-atomic-rmw to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 20:02):

abrown updated PR #4389 from isle-atomic-rmw to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 20:04):

abrown created PR review comment:

Ok, take a look at 3799054, which calculates the amode at this point instead of in emit.rs. I believe this is the more conventional way to do things in ISLE now, but unfortunately I see failures related reg.is_real() so the mem.finalize... part must not be right.

Wasmtime GitHub notifications bot (Jul 06 2022 at 20:04):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 20:06):

cfallin created PR review comment:

Ah, I think this is missing the corresponding use of the amode in the get_operands method; once that's added (in the same order as the allocs invocations) it should probably work?

Wasmtime GitHub notifications bot (Jul 06 2022 at 20:06):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 20:20):

abrown updated PR #4389 from isle-atomic-rmw to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 20:23):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 20:23):

abrown created PR review comment:

Hm, that fixes one problem but now I see wasm trap: out of bounds memory access from threads.wast. mem.get_operands() ends up doing a collector.reg_use() instead of collector.reg_late_use() like we were doing previously--seems like this could be the problem?

Wasmtime GitHub notifications bot (Jul 06 2022 at 21:52):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 21:52):

cfallin created PR review comment:

Ah yes! I think we'll need a variant of SyntheticAmode::get_operands() (and the functions it calls, Amode::get_operands() at least) that is something like get_operands_late() and internally does reg_late_use() instead.

Wasmtime GitHub notifications bot (Jul 06 2022 at 22:52):

abrown updated PR #4389 from isle-atomic-rmw to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 22:55):

abrown created PR review comment:

Ok, that seems to work.

Wasmtime GitHub notifications bot (Jul 06 2022 at 22:55):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 22:55):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 22:55):

abrown has marked PR #4389 as ready for review.

Wasmtime GitHub notifications bot (Jul 06 2022 at 22:57):

abrown updated PR #4389 from isle-atomic-rmw to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 22:59):

abrown updated PR #4389 from isle-atomic-rmw to main.

Wasmtime GitHub notifications bot (Jul 06 2022 at 23:02):

abrown has enabled auto merge for PR #4389.

Wasmtime GitHub notifications bot (Jul 06 2022 at 23:59):

abrown merged PR #4389.

Last updated: Nov 22 2024 at 17:03 UTC