afonso360 opened PR #3062 from interpreter-branches
to main
:
Hey, branches were causing the interpreter to crash.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Fallthrough will likely be removed together with the old backend framework.
afonso360 updated PR #3062 from interpreter-branches
to main
.
afonso360 updated PR #3062 from interpreter-branches
to main
.
afonso360 updated PR #3062 from interpreter-branches
to main
.
afonso360 updated PR #3062 from interpreter-branches
to main
.
afonso360 updated PR #3062 from interpreter-branches
to main
.
afonso360 updated PR #3062 from interpreter-branches
to main
.
abrown submitted PR review.
abrown submitted PR review.
abrown created PR review comment:
(But also looking at that issue, I must have thought when I wrote it that we could at least pass
b1
andb8
? Is that not the case anymore?)
abrown created PR review comment:
Hm, not totally sure about how to go about this; seems like there are a few options:
- keep this as you implemented it, which is a bit more verbose
- implement
into_bool
for integer types, but this may be a bit confusing because for otherinto_*
functions we error when the type doesn't match- implement a
ValueConversionKind::ToBoolean
and just usearg(0)?.convert(ValueConversionKind::ToBoolean)
here; I would probably lean this directionThoughts?
abrown created PR review comment:
@akirilov-arm, could you take a look at this? I think everything else makes sense but I don't know enough about the aarch64 backend to approve this part.
abrown created PR review comment:
For reference, the issue that fixes this is #2237.
abrown created PR review comment:
Ah, I see... I thought this meant "of" as in "a value of..." but I think it is an abbreviation of "overflows," right? Let's just go with the longer version to avoid confusion.
abrown created PR review comment:
BTW, I really appreciate you adding a bunch more CLIF tests!
cfallin submitted PR review.
cfallin created PR review comment:
Could you add some more explanation of this sequence?
I think I understand what's going on here -- we have a 0/1 flag generated by
cset
that is the actual overflow indicator, then we cause the main compare instruction to subtract that value fromINT32_MIN
, so we get an overflow (or underflow actually) on the subtract-as-compare. But it's somewhat difficult to work out!
afonso360 submitted PR review.
afonso360 created PR review comment:
implement into_bool for integer types, but this may be a bit confusing because for other into_* functions we error when the type doesn't match
I actually started by doing this, but it didn't feel "right" so I changed it.
Yeah, I think the
ValueConversionKind
is a good way to go, I'll update the PR
afonso360 created PR review comment:
Yes,
The "main" overflow check is the
sub
andcmp
with the extends, however this produces the result as ane
flag.However the main issue is that the caller of this function is expecting to use the
vs
flag, and we (currently) don't have a way to tell it to use another flag. So we set the overflow flag by subtracting either 1 / 0 from INT32_MIN.I'll update the comments to reflect this.
afonso360 submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
FWIW, that sounds like a reasonable refactor, if you'd prefer to go that way: return the (possibly modified) condition flag and use that in the caller. I think that at every place where we use
lower_icmp
, we have full control over what condition code we actually use, so this should be pretty reasonable, and would improve the generated code.
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
... and would also simplify the explanation that @cfallin has asked for :-). My vote would go towards refactoring as well.
afonso360 submitted PR review.
afonso360 created PR review comment:
Tested this again now, curiously, it works if we enable just the
interpret
test, but stops working with therun
test.test interpret test run target x86_64 machinst function %brnz_b1(b1) -> b1 { block0(v1: b1): brnz v1, block1 jump block2 block1: v2 = bconst.b1 true return v2 block2: v3 = bconst.b1 false return v3 } ; run: %brnz_b1(true) == true ; run: %brnz_b1(false) == false
We get:
Running `C:\Users\Afonso\CLionProjects\wasmtime\target\debug\clif-util.exe test ./test.clif` thread 'worker #0' panicked at 'argument type mismatch: b8 != b1', cranelift\filetests\src\function_runner.rs:200:13 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace FAIL ./test.clif: panicked in worker #0: argument type mismatch: b8 != b1 1 tests Error: 1 failure
afonso360 submitted PR review.
afonso360 created PR review comment:
Ok, I'll update into that model instead.
afonso360 updated PR #3062 from interpreter-branches
to main
.
afonso360 submitted PR review.
afonso360 created PR review comment:
I'm not 100% happy with how this turned out.
My issue is that this signature implies that we can change the output register, despite us never doing that. However changing
IcmpResult
into something like:enum IcmpResult { Flag(IntCC), Register, }
Also doesn't seem great, because now the output is inconsistent: if you request flag outputs you have to check the result and if you request register outputs you can ignore the result.
Do you guys have any suggestion on how to improve this?
This PR is also getting pretty big on the AArch64 side, if you would like me to separate it into 2 PR's let me know!
afonso360 updated PR #3062 from interpreter-branches
to main
.
afonso360 requested cfallin for a review on PR #3062.
afonso360 requested abrown for a review on PR #3062.
abrown submitted PR review.
abrown created PR review comment:
Well, I guess we'll just have to fix it all in #2237 :grinning_face_with_smiling_eyes:
abrown submitted PR review.
afonso360 updated PR #3062 from interpreter-branches
to main
.
afonso360 deleted PR review comment.
akirilov-arm submitted PR review.
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
At this point we should operate on
Cond
, notIntCC
. Also, let's call thisCondcode
instead ofFlag
.
akirilov-arm created PR review comment:
Can we add an assertion that we have not asked for
IcmpOutput::Flags
in this case?
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
Nit - the code structure looks like this:
... } else if !cond { ... } else { ...
Since you are changing the surrounding code significantly, could you also transform this, so that it does not use a negated condition:
... } else if cond { ... } else { ...
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
I think that there should be just a single block like this one, right before the function returns.
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
comparison
Also, there is no
NE
flag - it is calledZ
. However, a better way to phrase this is to sayThe result of this comparison is either the EQ or NE condition code ...
.
afonso360 submitted PR review.
afonso360 created PR review comment:
We should emit the correct results if the caller asked for flags, we emit a
subs
.Although, looking at this again, we can probably improve codegen by emitting a
sub
instead if the caller requestedRegisters
.I'll change that as well
afonso360 edited PR review comment.
afonso360 deleted PR review comment.
afonso360 updated PR #3062 from interpreter-branches
to main
.
afonso360 submitted PR review.
afonso360 created PR review comment:
We don't do this everywhere, namely I128 gt/lt/etc and vector icmps, so I don't know how well this ends up being in practice. But I pushed a commit with this at the end, let me know which way you prefer it.
afonso360 updated PR #3062 from interpreter-branches
to main
.
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
Are you saying that there are cases in which we pass
IcmpOutput::Flags
, but we return the result in a register (or vice versa)?
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
This should be
CondCode
as well, for consistency withIcmpResult
.
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
Actually they are not - there is a very small number of optional instructions that perform partial NZCV updates, but they are quite specialized, so I do not anticipate that we are ever going to need them in Cranelift.
That's why I also suggested the term "condition code" instead of "flag".
akirilov-arm edited PR review comment.
afonso360 submitted PR review.
afonso360 created PR review comment:
This is here to sort of warn callers that we may set flags that they do not expect, not that we really don't know what they are.
An example of this is the I128 EQ/NE implementation, we end that by emitting a
CMN
on the XOR of the inputs, this works for EQ/NE but if the caller tried to use any other cond code would result in wrong behaviour.But I think we can clarify this by saying that callers can check the output
cond
and its negative.
afonso360 edited PR review comment.
afonso360 edited PR review comment.
afonso360 submitted PR review.
afonso360 created PR review comment:
Thanks, that is much better!
afonso360 updated PR #3062 from interpreter-branches
to main
.
afonso360 edited PR review comment.
afonso360 requested akirilov-arm for a review on PR #3062.
akirilov-arm submitted PR review.
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
Typo -
comparison
.
akirilov-arm created PR review comment:
I just realized that this could be simplified further:
let mut should_materialize = output.reg().is_some();
akirilov-arm created PR review comment:
Typo -
comparisons
.
afonso360 updated PR #3062 from interpreter-branches
to main
.
akirilov-arm submitted PR review.
akirilov-arm created PR review comment:
Oh, right, a bit of tunnel vision on my side - I was absolutely concentrating on the flags case.
akirilov-arm submitted PR review.
abrown merged PR #3062.
Last updated: Dec 23 2024 at 12:05 UTC