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.
<!--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.
-->
bjorn3 created PR review comment:
Please push another type instead. I believe it tests multi return function signatures.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
This should list R32 too, right?
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Can bextend be replaced with uextend (for non-vector bools) and sextend (for vector bools)?
bjorn3 submitted PR review.
afonso360 submitted PR review.
afonso360 created PR review comment:
I'd be in favor of removing
bextend
andbreduce
. The latter can also be replaced byband_imm
.
afonso360 edited PR review comment.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt submitted PR review.
elliottt created PR review comment:
Thanks for catching that, I've switched it to
i8
.
elliottt submitted PR review.
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?
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
cfallin submitted PR review.
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 ittruthy
or somesuch?
cfallin created PR review comment:
Perhaps we could remove the
.as_bool()
helper altogether?
cfallin submitted PR review.
cfallin created PR review comment:
I don't think we want to delete the
Ireduce
andFdemote
cases here?
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...
cfallin created PR review comment:
Is this right? I would expect an
icmp
to give us ani8
with value either0
or1
, but here we're actually getting0
or-1
I think...
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.
cfallin submitted PR review.
elliottt created PR review comment:
Good catch, I got a bit overzealous there :)
elliottt submitted PR review.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt submitted PR review.
elliottt created PR review comment:
How would you feel about renaming it to
.as_truthy()
instead? It does convert types likeF32
toI32
, so it would be convenient to keep something like it around.
elliottt submitted PR review.
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 :)
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt submitted PR review.
elliottt created PR review comment:
I renamed it to
i8
, as that should hopefully give good documentation for the places that it's used.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt submitted PR review.
elliottt created PR review comment:
This was due to the implementation of
materialize_bool_result
in the aarch64 lowerings special-casing a result of1
bit to produce values of1
/0
, and otherwise producing-1
/0
. I've removed the special case, and it now unconditionally produces1
/0
. The downside of that change is that it complicates the lowering ofbmask
, but I think that's a reasonable trade-off.
cfallin submitted PR review.
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
).
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. Theruntests/sqrt.clif
test is a good example of this.
elliottt submitted PR review.
cfallin submitted PR review.
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!)
elliottt submitted PR review.
elliottt created PR review comment:
I've made this change, and removed/refactored similar extractors in the prelude.
elliottt submitted PR review.
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 theuextend
.
elliottt created PR review comment:
I really like using
uextend
/sextend
in place ofbextend
, and have made that refactoring. I've also removedbconst
,bextend
,breduce
andbint
, which has collapsed a lot of cases in the test suite.
elliottt submitted PR review.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
afonso360 submitted PR review.
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 ?
afonso360 edited PR review comment.
afonso360 edited PR review comment.
elliottt submitted PR review.
elliottt created PR review comment:
I love it! Much better :)
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
.
elliottt submitted PR review.
afonso360 submitted PR review.
afonso360 created PR review comment:
I don't think I can push directly to this branch, but feel free to make the change!
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt created PR review comment:
Pushed, thanks Afonso!
elliottt submitted PR review.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
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.
<!--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.
-->
elliottt edited PR #5031 from trevor/remove-booleans
to main
:
Remove the boolean types from cranelift, and the associated instructions
breduce
,bextend
,bconst
, andbint
. Standardize on using1
/0
for the return value from instructions that produce scalar boolean results, and-1
/0
for boolean vector elements.
<!--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.
-->
elliottt edited PR #5031 from trevor/remove-booleans
to main
:
Remove the boolean types from cranelift, and the associated instructions
breduce
,bextend
,bconst
, andbint
. Standardize on using1
/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.
-->
Last updated: Dec 23 2024 at 12:05 UTC