Stream: git-wasmtime

Topic: wasmtime / PR #6013 cranelift: Add narrower and wider con...

Wasmtime GitHub notifications bot (Mar 13 2023 at 23:27):

elliottt opened PR #6013 from trevor/narrower-instruction-constraint to main:

In service of merging #5947, add the narrower and wider constraints to the TypeVar type in the instruction DSL.

These two new constraints capture the situation where an instruction requires that its output be either narrower or wider than one of its input arguments. Instructions that fit one of these two patterns include: uextend, sextend, ireduce, fpromote, and fdemote. Previously these constraints were validated by special case typechecking behavior in the verifier, and with their addition those special cases can now be removed.

The benefit to #5947 is that the constraints are now available to the function_generator module when determining which arguments to select for random instruction instantiations. This gets us a bit closer to being able to use the generated opcode list as the source of truth for how we generate arbitrary instructions.

<!--

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 (Mar 13 2023 at 23:27):

elliottt requested jameysharp for a review on PR #6013.

Wasmtime GitHub notifications bot (Mar 13 2023 at 23:47):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Mar 13 2023 at 23:47):

elliottt created PR review comment:

This function doesn't capture the case where a variable depends on a parent that refines yet another parent. It might be worth thinking a bit about how this would work, though we don't currently have any instructions that combine constraints like that.

Wasmtime GitHub notifications bot (Mar 13 2023 at 23:49):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Mar 13 2023 at 23:49):

elliottt created PR review comment:

It might be worth adding a BitSet::singleton function to make it a bit more obvious what's going on here.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:12):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:12):

jameysharp created PR review comment:

I think this control flow is equivalent (since it's at the end of the loop where continue is implicit) and maybe easier to reason about.

        if let Some(tv) = typ.free_typevar() {
            if &tv != ctrl_typevar {
                return Err("type variable in output not derived from ctrl_typevar".into());
            }

            // Variables derived from the control variable are ok, but we remember if they refine
            // the control variable, as that still requires another type to instantiate.
            refines_output = typ.refines_parent();
        }

Also, how should refines_output be set if there are multiple output values? Should it be true if _any_ result type is a refinement?

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:12):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:12):

jameysharp created PR review comment:

This assertion message and the corresponding one for Wider don't reflect that you extended them to floats as well.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:12):

jameysharp created PR review comment:

I think, in conjunction with the good comment here mentioning 2^8, that it's more clear to just use the constant 8 as the upper bound:

                    tys.ints = BitSet::from_range(ctrl_type_bits as u8, 8);

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:12):

jameysharp created PR review comment:

Very nice to get rid of the lane_bits checks in the verifier. Can't we also get rid of the lane_count checks? And at that point delete the typecheck_special cases for the Unary format entirely?

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:46):

elliottt updated PR #6013 from trevor/narrower-instruction-constraint to main.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:52):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:52):

elliottt created PR review comment:

I had meant to do that, but apparently only got as far as the comment :sweat_smile:

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:54):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:54):

elliottt created PR review comment:

Hah, the only remaining checks were for vector arguments, and none of those instructions support vectors now. Great catch!

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:55):

elliottt updated PR #6013 from trevor/narrower-instruction-constraint to main.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:55):

elliottt updated PR #6013 from trevor/narrower-instruction-constraint to main.

Wasmtime GitHub notifications bot (Mar 14 2023 at 00:55):

elliottt has marked PR #6013 as ready for review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:00):

elliottt updated PR #6013 from trevor/narrower-instruction-constraint to main.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:01):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:01):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:01):

jameysharp created PR review comment:

I'm still wondering how this should work if the instruction has multiple value results. Right now, refines_output is set by the _last_ value result, which doesn't seem right. Should it be set true if _any_ value result is a refinement?

            if typ.refines_parent() {
                refines_output = true;
            }

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:01):

jameysharp created PR review comment:

I snuck a variable inlining in there so this let-binding should be dead now :sweat_smile:

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:04):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:04):

elliottt created PR review comment:

Yep, I think you're correct: it should be true if any result is a refinement. Thanks for catching this!

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:05):

elliottt updated PR #6013 from trevor/narrower-instruction-constraint to main.

Wasmtime GitHub notifications bot (Mar 14 2023 at 01:06):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Mar 14 2023 at 16:05):

elliottt updated PR #6013 from trevor/narrower-instruction-constraint to main.

Wasmtime GitHub notifications bot (Mar 14 2023 at 17:15):

elliottt merged PR #6013.

Last updated: Dec 23 2024 at 12:05 UTC