Stream: git-wasmtime

Topic: wasmtime / PR #4545 x64: Migrate trapif to ISLE

Wasmtime GitHub notifications bot (Jul 27 2022 at 23:36):

elliottt opened PR #4545 from trevor/x64-lower-trapif to main:

• Add a ProducesBool type for capturing comparisons
• Migrate trapif to ISLE

<!--

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 27 2022 at 23:38):

elliottt edited PR #4545 from trevor/x64-lower-trapif to main:

Migrate the trapif instruction to ISLE, and extract the lowering of icmp for scalars to a utility function. The icmp extraction will be helpful for future instruction migrations, as it mirrors the emit_cmp instruction in the rust implementation.

<!--

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 27 2022 at 23:39):

elliottt edited PR #4545 from trevor/x64-lower-trapif to main:

Migrate the trapif instruction to ISLE, and extract the lowering of icmp for scalars to a utility function. The icmp extraction will be helpful for future instruction migrations, as it mirrors the emit_cmp instruction in the rust implementation.

I'd recommend reviewing by commit, and turning off whitespace in the diff.

<!--

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 27 2022 at 23:42):

elliottt updated PR #4545 from trevor/x64-lower-trapif to main.

Wasmtime GitHub notifications bot (Jul 27 2022 at 23:46):

elliottt updated PR #4545 from trevor/x64-lower-trapif to main.

Wasmtime GitHub notifications bot (Jul 27 2022 at 23:51):

elliottt edited PR #4545 from trevor/x64-lower-trapif to main:

Migrate the trapif instruction to ISLE, and extract the lowering of icmp for scalars to a utility function. The icmp extraction will be helpful for future instruction migrations, as its uses mirror the existing emit_cmp instruction in the rust implementation.

I'd recommend reviewing by commit, and turning off whitespace in the diff.

<!--

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 28 2022 at 01:54):

elliottt updated PR #4545 from trevor/x64-lower-trapif to main.

Wasmtime GitHub notifications bot (Jul 28 2022 at 04:00):

elliottt requested cfallin for a review on PR #4545.

Wasmtime GitHub notifications bot (Jul 28 2022 at 04:00):

elliottt has marked PR #4545 as ready for review.

Wasmtime GitHub notifications bot (Jul 28 2022 at 20:14):

elliottt updated PR #4545 from trevor/x64-lower-trapif to main.

Wasmtime GitHub notifications bot (Jul 28 2022 at 22:33):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2022 at 22:33):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2022 at 22:33):

cfallin created PR review comment:

Hmm -- I'm not sure about this: we're matching that iadd_ifcout is our input, but we don't actually use the input, rather we implicitly depend on it clobbering flags. This then breaks our producer-consumer coupling as well, necessitating the emit_consumer helper, which feels pretty unsafe to me.

The most immediate issue is that if the iadd_ifcout isn't otherwise used, it won't actually be lowered, and then we're going to use bogus flags to do our trap. That seems like the biggest issue. I suspect this case simply isn't covered in our existing tests.

More broadly, we want to remove flags types competely eventually, because they're pretty error-prone (see #3249). But until then, we should work on doing something safer (at least correct, ideally more elegant as well!).

What about this: we write a constructor that takes the flags value (so in the pattern, capture it with flags @ (iadd_ifcout _ _)), and produces a ProducesFlags.AlreadyExistingFlags (that contains no actual inst), and handle that in with_flags. This ctor will need to call out to an external constructor to bump the refcount on the iadd_ifcout as well (normally put_in_regs does this in an implicit converter from Value to Reg). Let's add a test where the iadd_ifcout` is used only for its flags, and make sure it lowers properly.

There's also a relevant comment near the lowering rules for iadd_ifcout that should be updated I think.

Thoughts?

Wasmtime GitHub notifications bot (Jul 29 2022 at 00:25):

elliottt updated PR #4545 from trevor/x64-lower-trapif to main.

Last updated: Nov 22 2024 at 17:03 UTC