Stream: git-wasmtime

Topic: wasmtime / PR #2468 Improve usability of constants

Wasmtime GitHub notifications bot (Dec 02 2020 at 22:48):

abrown requested cfallin and bnjbvr for a review on PR #2468.

Wasmtime GitHub notifications bot (Dec 02 2020 at 22:48):

abrown opened PR #2468 from improve-constants to main:

This adds a higher-level method, LowerCtx::emit_constant for lowering a constant in one of two ways:

• using a DataValue (instead of the previous u64), a value can be generated into a register using MachInst::gen_constant
• alternately, if MachInst::should_genenerate_constant_in_pool is true, then a value is emitted into the constant pool/island and loaded from memory at runtime; this path uses MachInst::gen_constant_in_pool.

<!--

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 (Dec 02 2020 at 22:48):

abrown requested cfallin and bnjbvr for a review on PR #2468.

Wasmtime GitHub notifications bot (Dec 02 2020 at 23:01):

abrown updated PR #2468 from improve-constants to main:

This adds a higher-level method, LowerCtx::emit_constant for lowering a constant in one of two ways:

• using a DataValue (instead of the previous u64), a value can be generated into a register using MachInst::gen_constant
• alternately, if MachInst::should_genenerate_constant_in_pool is true, then a value is emitted into the constant pool/island and loaded from memory at runtime; this path uses MachInst::gen_constant_in_pool.

<!--

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 (Dec 02 2020 at 23:41):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 02 2020 at 23:41):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 02 2020 at 23:41):

cfallin created PR Review Comment:

s/in place/into a constant pool/, no? (Or perhaps "should be generated into a constant pool and loaded (if true); otherwise, it should be generated in place by an instruction or instruction sequence."

We should also clarify that if this function returns false, then gen_constant must be able to handle this value, and if true, then gen_constant_in_pool should.

Wasmtime GitHub notifications bot (Dec 02 2020 at 23:41):

cfallin created PR Review Comment:

Perhaps we can call this gen_constant_in_place now (analogous to ..._in_pool below)?

Wasmtime GitHub notifications bot (Dec 08 2020 at 15:02):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Dec 08 2020 at 15:02):

bnjbvr created PR Review Comment:

Looks like there's a solution involving more traits that could avoid the assertions and the dummy unimplemented:

• have a trait ConstantGenerator that has a method "gen_constant"
• have a trait PooledConstantGenerator, that has three methods "gen_constant_without_pool", "gen_constant_with_pool", "must_generate_in_pool"
• then PooledConstantGenerator gets an blanket impl of ConstantGenerator based on these three functions above.

Then backends have to implement ConstantGenerator either directly, or via an opt-in choice to use the pooled variant. It's a bit more involved though...

Wasmtime GitHub notifications bot (Dec 08 2020 at 15:02):

bnjbvr edited PR Review Comment.

Wasmtime GitHub notifications bot (Dec 08 2020 at 15:16):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Dec 08 2020 at 18:20):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 08 2020 at 18:20):

cfallin created PR Review Comment:

I like this scheme -- @abrown, does this seem reasonable to incorporate in your PR? I'd be happy to review it as a refactoring followup otherwise.

Wasmtime GitHub notifications bot (Dec 08 2020 at 18:41):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Dec 08 2020 at 18:41):

abrown created PR Review Comment:

I like it in principle! Let me dive back in and see how it comes out in practice.

Wasmtime GitHub notifications bot (Feb 10 2023 at 18:56):

abrown closed without merge PR #2468.

Last updated: Nov 22 2024 at 16:03 UTC