Stream: git-wasmtime

Topic: wasmtime / PR #3887 Add vector compare to 0 optims

Wasmtime GitHub notifications bot (Mar 07 2022 at 12:58):

FreddieLiardet opened PR #3887 from vec_comp to main:

Signed-off-by: Freddie Liardet <frederick.liardet@arm.com>

Optimization for simd comparisons to 0 vectors using dedicated 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 07 2022 at 18:17):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2022 at 18:17):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 07 2022 at 18:17):

cfallin created PR review comment:

It's a tiny stylistic nit but I'd prefer to avoid this paren on its own line style; the more common S-expr formatting is something like

(let ((x ...)
      (y ...)
      (z ...))
  body)

Wasmtime GitHub notifications bot (Mar 07 2022 at 18:17):

cfallin created PR review comment:

The def_inst can be skipped now that we have implicit conversions -- should make this pattern look much nicer!

Wasmtime GitHub notifications bot (Mar 07 2022 at 18:17):

cfallin created PR review comment:

Could you add spaces between the term name and arg list parenthesis here?

Wasmtime GitHub notifications bot (Mar 07 2022 at 18:17):

cfallin created PR review comment:

put_in_reg also has an implicit conversion defined now, so we can do:

  (rn Reg x)

or just use rn in a context that expects a Reg and things should Just Work.

Wasmtime GitHub notifications bot (Mar 07 2022 at 18:17):

cfallin created PR review comment:

Tiny nit but we've mostly stuck to all-lowercase with underscores ("snake case"); would prefer noteq or not_eq here.

Wasmtime GitHub notifications bot (Mar 07 2022 at 18:17):

cfallin created PR review comment:

It looks like this isn't changing the matched IntCC; could we write this as just IntCC::Equal | IntCC::SignedGreaterThanOrEqual | ... => Some(cond)?

Wasmtime GitHub notifications bot (Mar 07 2022 at 18:17):

cfallin created PR review comment:

Could you add some comments here describing what the extractors look for? E.g. from icmp_cond_zero it's not entirely clear to me what this means: a "zero condition"? Does it change the IntCC somehow?

Wasmtime GitHub notifications bot (Mar 07 2022 at 18:17):

cfallin created PR review comment:

a more descriptive name might be good for this helper: float_cc_cmp_zero_to_vec_misc_op maybe? fcompare doesn't tell the reader what it does, other than suggest perhaps it returns an instruction or something to do a compare... also, the name does not at all suggest the connection to zero.

Wasmtime GitHub notifications bot (Mar 08 2022 at 13:50):

FreddieLiardet updated PR #3887 from vec_comp to main.

Wasmtime GitHub notifications bot (Mar 08 2022 at 17:50):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 08 2022 at 17:50):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 08 2022 at 17:50):

cfallin created PR review comment:

Sorry, I should've said "and similar below" to earlier comments about naming the above constructors. Could you make all of these added ones more descriptive as well? E.g. f_comp_inst doesn't give me any hint that this might have to do with zero comparisons; at best I might buess that it means "create a floating point comparison instruction", which sounds like a generic constructor.

Doc comments on each (one line is fine) to say what they do would also be helpful. Thanks!

Wasmtime GitHub notifications bot (Mar 08 2022 at 17:50):

cfallin created PR review comment:

More paren-style comments: here and below, the closing parens should be on the last line of the body, not on separate lines.

Wasmtime GitHub notifications bot (Mar 09 2022 at 15:44):

FreddieLiardet updated PR #3887 from vec_comp to main.

Wasmtime GitHub notifications bot (Mar 10 2022 at 00:19):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 10 2022 at 00:20):

cfallin merged PR #3887.

Last updated: Nov 22 2024 at 16:03 UTC