Stream: git-wasmtime

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


view this post on Zulip 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.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2022 at 18:17):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 07 2022 at 18:17):

cfallin submitted PR review.

view this post on Zulip 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)

view this post on Zulip 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!

view this post on Zulip 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?

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip 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)?

view this post on Zulip 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?

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2022 at 13:50):

FreddieLiardet updated PR #3887 from vec_comp to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2022 at 17:50):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 08 2022 at 17:50):

cfallin submitted PR review.

view this post on Zulip 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!

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 09 2022 at 15:44):

FreddieLiardet updated PR #3887 from vec_comp to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2022 at 00:19):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2022 at 00:20):

cfallin merged PR #3887.


Last updated: Dec 23 2024 at 13:07 UTC