Stream: git-wasmtime

Topic: wasmtime / PR #5031 cranelift: Remove booleans

Wasmtime GitHub notifications bot (Oct 12 2022 at 17:17):

elliottt edited PR #5031 from trevor/remove-booleans to main:

Remove the boolean types from cranelift, and the associated instructions breduce, bextend, bconst, and bint. Standardize on using 1/0 for the return value from instructions that produce scalar boolean results, and -1/0 for boolean vector elements.

Fixes #3205
<!--

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 (Oct 12 2022 at 19:18):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 12 2022 at 19:59):

elliottt has marked PR #5031 as ready for review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 20:00):

elliottt requested cfallin for a review on PR #5031.

Wasmtime GitHub notifications bot (Oct 12 2022 at 22:31):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 12 2022 at 22:48):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 22:48):

cfallin created PR review comment:

we can probably call this file something other than b1.clif now -- maybe conditional_values.clif or something?

Wasmtime GitHub notifications bot (Oct 12 2022 at 22:48):

cfallin created PR review comment:

s/false/0/ here (or "and require the return value to be nonzero")?

Wasmtime GitHub notifications bot (Oct 12 2022 at 22:48):

cfallin created PR review comment:

So nice to see this mess of canonicalization and type-coercion go away!

Wasmtime GitHub notifications bot (Oct 12 2022 at 22:48):

cfallin created PR review comment:

I think this could get a bit better (one instruction shorter) with

tst w0, #255
csel ...

(tst w0, #255 is an alias for ands wzr, w0, #255; if we don't have machinst support for that yet it should be easy as another ALU op, one bit different from and)

Wasmtime GitHub notifications bot (Oct 12 2022 at 22:48):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 23:13):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 23:13):

elliottt created PR review comment:

Great point. I like the latter phrasing, and will make that change.

Wasmtime GitHub notifications bot (Oct 12 2022 at 23:36):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 23:36):

elliottt created PR review comment:

Good point, the existing name isn't really all that meaningful going forward :)

Wasmtime GitHub notifications bot (Oct 12 2022 at 23:47):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 23:47):

elliottt created PR review comment:

Would you be okay with addressing this one in follow-up PR?

Wasmtime GitHub notifications bot (Oct 12 2022 at 23:49):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 12 2022 at 23:54):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 23:54):

cfallin created PR review comment:

Yep, that's fine!

Wasmtime GitHub notifications bot (Oct 13 2022 at 01:52):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 13 2022 at 01:52):

elliottt created PR review comment:

I went ahead and fixed it on this branch, and found a bug with the x64 lowering of select in the process!

Wasmtime GitHub notifications bot (Oct 13 2022 at 01:56):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 13 2022 at 05:17):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 13 2022 at 05:17):

cfallin created PR review comment:

while here, a useful runtest to add might be a very slight variation on this one that intentionally tries to have nonzero bits in the upper undef bits of the i8-in-register, to make sure we're really testing just the low 8 bits; for example, by adding and wrapping:

block0(v0: i8):
  v1 = iconst.i8 255
  v2 = iadd.i8 v0, v1
  brz v2, ...

then it should branch for input arg v0 = 1 even though the register will likely contain 256...

Wasmtime GitHub notifications bot (Oct 13 2022 at 05:17):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 13 2022 at 05:17):

cfallin created PR review comment:

Good catch! Just to make sure, do we have a runtest somewhere that tests with a nonzero-but-even value to catch LSB-only checks like this?

Wasmtime GitHub notifications bot (Oct 13 2022 at 09:21):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Oct 13 2022 at 09:21):

uweigand created PR review comment:

Hmm. This helper is only called for possible scalar result types of icmp and fcmp. These are specified as &Int.as_bool() and &Float.as_bool() so I thought I needed to handle multiple widths here. But looking at this again, actually the only scalar return type of as_bool has always been $B1 (and is now $I8), so it seems this is only type we need to handle here in the first place.

Wasmtime GitHub notifications bot (Oct 13 2022 at 09:21):

uweigand submitted PR review.

Wasmtime GitHub notifications bot (Oct 13 2022 at 10:37):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 13 2022 at 10:37):

afonso360 created PR review comment:

;; For conversions that fit in a register, we can use csetm.

Wasmtime GitHub notifications bot (Oct 13 2022 at 10:37):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 13 2022 at 16:26):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 13 2022 at 16:27):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 13 2022 at 16:29):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 13 2022 at 16:43):

elliottt created PR review comment:

We do in the runtests: there's a case that checks the return value with 2 as the argument to select in runtests/br.clif.

Wasmtime GitHub notifications bot (Oct 13 2022 at 16:43):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 13 2022 at 16:44):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 13 2022 at 16:44):

elliottt created PR review comment:

That's a great idea, I'll add that test!

Wasmtime GitHub notifications bot (Oct 13 2022 at 17:25):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 13 2022 at 22:07):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 13 2022 at 23:28):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 17 2022 at 21:07):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 17 2022 at 21:25):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 17 2022 at 21:39):

elliottt updated PR #5031 from trevor/remove-booleans to main.

Wasmtime GitHub notifications bot (Oct 17 2022 at 23:00):

elliottt merged PR #5031.

Last updated: Nov 22 2024 at 17:03 UTC