Stream: git-wasmtime

Topic: wasmtime / PR #4562 Allow multiple `_` definitions in a `...

Wasmtime GitHub notifications bot (Jul 29 2022 at 23:31):

elliottt opened PR #4562 from trevor/isle-wildcard-let to main:

• Allow multiple definitions of _ in let
• Refactor now that multiple _ definitions are allowed

<!--

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 29 2022 at 23:36):

elliottt updated PR #4562 from trevor/isle-wildcard-let to main.

Wasmtime GitHub notifications bot (Jul 29 2022 at 23:44):

elliottt updated PR #4562 from trevor/isle-wildcard-let to main.

Wasmtime GitHub notifications bot (Jul 29 2022 at 23:53):

elliottt edited PR #4562 from trevor/isle-wildcard-let to main:

On main a let expression in ISLE currently requires unique names for each declaration. If you are sequencing a number of effects together using a let expression, you'll end up with multiple declarations whose name you don't care about, like in this example from the s390x backend: https://github.com/bytecodealliance/wasmtime/blob/8e9e9c52a1da9c661ad8a25210e38826bf8f7362/cranelift/codegen/src/isa/s390x/inst.isle#L3057-L3064.

This PR allows multiple definitions with the name _ in a let expression, removing the need to come up with unique names. The previous example can be refactored to the following after this change:

;; Use a boolean condition to select between two registers.
(decl select_bool_reg (Type ProducesBool Reg Reg) Reg)
(rule (select_bool_reg ty (ProducesBool.ProducesBool producer cond) reg_true reg_false)
      (let ((dst WritableReg (temp_writable_reg ty))
            (_ Unit (emit_producer producer))
            (_ Unit (emit_mov ty dst reg_false))
            (_ Unit (emit_consumer (emit_cmov_reg ty dst cond reg_true))))
        dst))

<!--

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 29 2022 at 23:53):

elliottt edited PR #4562 from trevor/isle-wildcard-let to main:

Currently, a let expression in ISLE currently requires unique names for each declaration. If you are sequencing a number of effects together using a let expression, you'll end up with multiple declarations whose name you don't care about, like in this example from the s390x backend: https://github.com/bytecodealliance/wasmtime/blob/8e9e9c52a1da9c661ad8a25210e38826bf8f7362/cranelift/codegen/src/isa/s390x/inst.isle#L3057-L3064.

This PR allows multiple definitions with the name _ in a let expression, removing the need to come up with unique names. The previous example can be refactored to the following after this change:

;; Use a boolean condition to select between two registers.
(decl select_bool_reg (Type ProducesBool Reg Reg) Reg)
(rule (select_bool_reg ty (ProducesBool.ProducesBool producer cond) reg_true reg_false)
      (let ((dst WritableReg (temp_writable_reg ty))
            (_ Unit (emit_producer producer))
            (_ Unit (emit_mov ty dst reg_false))
            (_ Unit (emit_consumer (emit_cmov_reg ty dst cond reg_true))))
        dst))

<!--

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 29 2022 at 23:54):

elliottt edited PR #4562 from trevor/isle-wildcard-let to main:

Currently, a let expression in ISLE currently requires unique names for each declaration. If you are sequencing a number of effects together using a let expression, you'll end up with multiple declarations whose results you don't need. Here's an example of this from the s390x backend: https://github.com/bytecodealliance/wasmtime/blob/8e9e9c52a1da9c661ad8a25210e38826bf8f7362/cranelift/codegen/src/isa/s390x/inst.isle#L3057-L3064

This PR allows multiple definitions with the name _ in a let expression, removing the need to come up with unique names. The previous example can be refactored to the following after this change:

;; Use a boolean condition to select between two registers.
(decl select_bool_reg (Type ProducesBool Reg Reg) Reg)
(rule (select_bool_reg ty (ProducesBool.ProducesBool producer cond) reg_true reg_false)
      (let ((dst WritableReg (temp_writable_reg ty))
            (_ Unit (emit_producer producer))
            (_ Unit (emit_mov ty dst reg_false))
            (_ Unit (emit_consumer (emit_cmov_reg ty dst cond reg_true))))
        dst))

<!--

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 (Aug 01 2022 at 16:24):

elliottt has marked PR #4562 as ready for review.

Wasmtime GitHub notifications bot (Aug 01 2022 at 18:00):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2022 at 19:21):

elliottt updated PR #4562 from trevor/isle-wildcard-let to main.

Wasmtime GitHub notifications bot (Aug 01 2022 at 19:25):

elliottt requested cfallin for a review on PR #4562.

Wasmtime GitHub notifications bot (Aug 01 2022 at 19:34):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2022 at 19:34):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2022 at 19:34):

cfallin created PR review comment:

Would you mind turning at least this case into a run-test and invoking the rule from a test toplevel, asserting that the most recent binding wins?

Wasmtime GitHub notifications bot (Aug 01 2022 at 20:13):

elliottt updated PR #4562 from trevor/isle-wildcard-let to main.

Wasmtime GitHub notifications bot (Aug 01 2022 at 20:26):

elliottt updated PR #4562 from trevor/isle-wildcard-let to main.

Wasmtime GitHub notifications bot (Aug 01 2022 at 20:28):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Aug 01 2022 at 20:28):

elliottt created PR review comment:

I reworked it as a link test in the isle/isle_examples/link directory, and modified the test runner to also execute the output of rustc on these tests.

Last updated: Nov 22 2024 at 16:03 UTC