wasmtime / PR #5031 Cranelift: Remove booleans · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #5031 Cranelift: Remove booleans

Wasmtime GitHub notifications bot (Oct 07 2022 at 00:33):

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

WIP PR to remove the boolean types from cranelift, in favor of using the int types instead.

Wasmtime GitHub notifications bot (Oct 07 2022 at 08:23):

bjorn3 created PR review comment:

Please push another type instead. I believe it tests multi return function signatures.

Wasmtime GitHub notifications bot (Oct 07 2022 at 08:23):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 07 2022 at 08:26):

bjorn3 created PR review comment:

This should list R32 too, right?

Wasmtime GitHub notifications bot (Oct 07 2022 at 08:26):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 07 2022 at 08:29):

bjorn3 created PR review comment:

Can bextend be replaced with uextend (for non-vector bools) and sextend (for vector bools)?

Wasmtime GitHub notifications bot (Oct 07 2022 at 08:29):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 07 2022 at 15:33):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 07 2022 at 15:33):

afonso360 created PR review comment:

I'd be in favor of removing bextend and breduce. The latter can also be replaced by band_imm.

Wasmtime GitHub notifications bot (Oct 07 2022 at 15:33):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 07 2022 at 16:25):

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

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

elliottt submitted PR review.

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

elliottt created PR review comment:

Thanks for catching that, I've switched it to i8.

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

elliottt submitted PR review.

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

elliottt created PR review comment:

Probably, though I'm not sure we should address that on this branch. Would you like to add it in a separate PR?

Wasmtime GitHub notifications bot (Oct 07 2022 at 16:59):

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

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

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

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

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

Wasmtime GitHub notifications bot (Oct 07 2022 at 18:10):

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

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

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

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:19):

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

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

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

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:38):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:38):

cfallin created PR review comment:

Do we still need a typevar named b1 at all? Is this used for "truthy" things like icmp return values etc? If so, perhaps we could call it truthy or somesuch?

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:38):

cfallin created PR review comment:

Perhaps we could remove the .as_bool() helper altogether?

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:38):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:38):

cfallin created PR review comment:

I don't think we want to delete the Ireduce and Fdemote cases here?

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:38):

cfallin created PR review comment:

We can probably remove this extractor now and just use a constant $I64, no? That has the nice extra benefit of allowing more precise overlap checking...

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:38):

cfallin created PR review comment:

Is this right? I would expect an icmp to give us an i8 with value either 0 or 1, but here we're actually getting 0 or -1 I think...

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:39):

cfallin created PR review comment:

Yep, if we no longer have bools then these instructions no longer make sense; the user can zero- or sign-extend an integer (which happens to be a 0/1 or 0/-1 "truthy" value) in the usual ways.

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:39):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:48):

elliottt created PR review comment:

Good catch, I got a bit overzealous there :)

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:48):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 07 2022 at 23:51):

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

Wasmtime GitHub notifications bot (Oct 08 2022 at 00:55):

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

Wasmtime GitHub notifications bot (Oct 08 2022 at 01:15):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 08 2022 at 01:15):

elliottt created PR review comment:

How would you feel about renaming it to .as_truthy() instead? It does convert types like F32 to I32, so it would be convenient to keep something like it around.

Wasmtime GitHub notifications bot (Oct 08 2022 at 01:16):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 08 2022 at 01:16):

elliottt created PR review comment:

That's a great point! I hadn't started thinking about improvements that we could make without the boolean types, and was just in "make it compile" mode instead :)

Wasmtime GitHub notifications bot (Oct 08 2022 at 01:22):

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

Wasmtime GitHub notifications bot (Oct 08 2022 at 01:22):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 08 2022 at 01:22):

elliottt created PR review comment:

I renamed it to i8, as that should hopefully give good documentation for the places that it's used.

Wasmtime GitHub notifications bot (Oct 10 2022 at 20:47):

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

Wasmtime GitHub notifications bot (Oct 10 2022 at 21:10):

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

Wasmtime GitHub notifications bot (Oct 10 2022 at 21:27):

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

Wasmtime GitHub notifications bot (Oct 10 2022 at 23:34):

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

Wasmtime GitHub notifications bot (Oct 11 2022 at 00:50):

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

Wasmtime GitHub notifications bot (Oct 11 2022 at 16:11):

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

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:17):

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

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:24):

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

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:27):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:27):

elliottt created PR review comment:

This was due to the implementation of materialize_bool_result in the aarch64 lowerings special-casing a result of 1 bit to produce values of 1/0, and otherwise producing -1/0. I've removed the special case, and it now unconditionally produces 1/0. The downside of that change is that it complicates the lowering of bmask, but I think that's a reasonable trade-off.

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:30):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:30):

cfallin created PR review comment:

Cool, makes sense, thanks!

Do we have any runtests that return the result of an icmp or friends and directly assert its value? Seems like if not, that would be good coverage: value is 0 or 1 for various cases for each kind of comparison (icmp, fcmp).

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:35):

elliottt created PR review comment:

We have a lot of tests that perform a comparison, uextend it to a larger type, and then test the resulting values. The runtests/sqrt.clif test is a good example of this.

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:35):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:36):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:36):

cfallin created PR review comment:

Awesome, we're covered then, thanks! (I guess I should just review the tests in general; still need to do that, sorry!)

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:37):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:37):

elliottt created PR review comment:

I've made this change, and removed/refactored similar extractors in the prelude.

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:51):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:51):

elliottt created PR review comment:

It might be worth refactoring those tests to return the value of the icmp/fcmp directly, as there might be interference from the uextend.

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:53):

elliottt created PR review comment:

I really like using uextend/sextend in place of bextend, and have made that refactoring. I've also removed bconst, bextend, breduce and bint, which has collapsed a lot of cases in the test suite.

Wasmtime GitHub notifications bot (Oct 11 2022 at 18:53):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 11 2022 at 22:50):

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

Wasmtime GitHub notifications bot (Oct 12 2022 at 01:40):

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

Wasmtime GitHub notifications bot (Oct 12 2022 at 10:55):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 10:55):

afonso360 created PR review comment:

I was playing around with this, and I think I got something slightly shorter, what do you think about this: https://github.com/bytecodealliance/wasmtime/commit/51baa19adfdabb9a31b0465c796ffad05bfaa0c3 ?

Wasmtime GitHub notifications bot (Oct 12 2022 at 11:02):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 12 2022 at 11:05):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:06):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:06):

elliottt created PR review comment:

I love it! Much better :)

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:14):

elliottt created PR review comment:

Would you like to push that commit to this branch, or should I make the change? Either way, I'll make sure to list you in the commit as co-authored-by.

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:14):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:15):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:15):

afonso360 created PR review comment:

I don't think I can push directly to this branch, but feel free to make the change!

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:35):

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

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:35):

elliottt created PR review comment:

Pushed, thanks Afonso!

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:35):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:55):

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

Wasmtime GitHub notifications bot (Oct 12 2022 at 16:57):

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

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

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

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

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

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

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.

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

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.

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

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

Last updated: Nov 22 2024 at 16:03 UTC