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.
-->
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
elliottt has marked PR #5031 as ready for review.
elliottt requested cfallin for a review on PR #5031.
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
cfallin submitted PR review.
cfallin created PR review comment:
we can probably call this file something other than
b1.clif
now -- maybeconditional_values.clif
or something?
cfallin created PR review comment:
s/false/0/ here (or "and require the return value to be nonzero")?
cfallin created PR review comment:
So nice to see this mess of canonicalization and type-coercion go away!
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 forands wzr, w0, #255
; if we don't have machinst support for that yet it should be easy as another ALU op, one bit different fromand
)
cfallin submitted PR review.
elliottt submitted PR review.
elliottt created PR review comment:
Great point. I like the latter phrasing, and will make that change.
elliottt submitted PR review.
elliottt created PR review comment:
Good point, the existing name isn't really all that meaningful going forward :)
elliottt submitted PR review.
elliottt created PR review comment:
Would you be okay with addressing this one in follow-up PR?
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
cfallin submitted PR review.
cfallin created PR review comment:
Yep, that's fine!
elliottt submitted PR review.
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!
elliottt updated PR #5031 from trevor/remove-booleans
to main
.
cfallin submitted PR review.
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 contain256
...
cfallin submitted PR review.
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?
uweigand submitted PR review.
uweigand created PR review comment:
Hmm. This helper is only called for possible scalar result types of
icmp
andfcmp
. 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 ofas_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.
uweigand submitted PR review.
afonso360 submitted PR review.
afonso360 created PR review comment:
;; For conversions that fit in a register, we can use csetm.
afonso360 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 created PR review comment:
We do in the runtests: there's a case that checks the return value with
2
as the argument toselect
inruntests/br.clif
.
elliottt submitted PR review.
elliottt submitted PR review.
elliottt created PR review comment:
That's a great idea, I'll add that test!
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 merged PR #5031.
Last updated: Dec 23 2024 at 12:05 UTC