elliottt opened PR #5446 from trevor/brif
to main
:
Add a conditional branch instruction with two targets:
brif
. This instruction
will eventually replacebrz
andbrnz
, as it encompasses the behavior of
both.
<!--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 #5446 from trevor/brif
to main
:
Add a conditional branch instruction with two targets:
brif
. This instruction will eventually replacebrz
andbrnz
, as it encompasses the behavior of both.
<!--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 #5446 from trevor/brif
to main
.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
This is a gross hack.
Block
andValue
are two entirely distinct types. They should never be mixed.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Did you mean to commit this file?
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Maybe you could have a
BlockOrValue
type just forBlockWithArgs
and then ensure that this type never leaks out of theBlockWithArgs
?
elliottt submitted PR review.
elliottt created PR review comment:
It was pulled in after rebasing over more recent work. Thanks for catching that.
elliottt updated PR #5446 from trevor/brif
to main
.
jameysharp submitted PR review.
jameysharp created PR review comment:
I appreciate how quick you are to share your insights on Cranelift issues and PRs. Your comments are usually helpful and lead to making Cranelift better for everyone. In this case, though, this is not the most helpful comment.
Before commenting on code where I'm not satisfied with its quality, I try to ask myself: Why did the author choose this way of expressing this idea? Maybe they know it's not ideal but they needed a placeholder while they're focusing on other parts of the problem. This case looks like a placeholder, given the empty "TODO" comments. Since this PR is still in "draft" status, placeholders are fine. They're only an issue if we merge something to main that'll cause us problems later.
It's useful to open draft PRs to run CI against work-in-progress. They're also an opportunity to get early feedback, but preferably when the author has specifically requested it. Even then, that feedback should be focused on the high-level ideas, not the implementation details, which are likely to change as the patch matures. (I aspire to follow the "Three-Phase Contribution Review" model described in "The Gentle Art of Patch Review".)
So, while I agree with you that this PR probably shouldn't be merged in exactly this form, it's too early to critique this implementation detail with the degree of severity of your first reaction. Your
BlockOrValue
suggestion is a good one, but Trevor may have equally good alternatives in mind.It's good to suggest better alternatives, and even good to note concerns. For example, you could say, "We'll have to carefully audit anywhere that the
Block
andValue
types are allowed to mix, and in the final version of this PR I'd prefer to avoid that." Just keep in mind that the patch's author may already know that, especially for draft PRs, and that they may even have good reasons for it.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
I agree I could have reacted much better. I will keep this in mind for the future.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
jameysharp submitted PR review.
jameysharp created PR review comment:
"bloks" -> "blocks"
jameysharp created PR review comment:
I've noticed rust-analyzer doesn't do quite what I want with imports like this. I'd want
BlockWithArgs
to go into the list of imports on line 68, and leaveuse crate::ir;
as a separate item, but several times I've seen it do this kind of rewrite instead.
jameysharp created PR review comment:
I haven't looked at how this lexer works. Does this correctly handle missing whitespace? Like, if someone wrote
brif v0 thenblock1 elseblock2
, would it reject that or would it strip off the "then"/"else" prefix and then see that it has a valid block name?
jameysharp created PR review comment:
I think this is the first place that
typecheck_variable_args_iterator
is called twice with the same instruction. Since it compares the given iterator againstinst_variable_args(inst)
, I wouldn't think that would be the right thing to do. Is it?
jameysharp created PR review comment:
I see this was copy-pasted from the comment above because they all have the same typo. :laughing: "primarliy" -> "primarily"
jameysharp submitted PR review.
elliottt created PR review comment:
What about adding a
self
import on line 67 and removing line 66?
elliottt submitted PR review.
elliottt edited PR review comment.
jameysharp submitted PR review.
jameysharp created PR review comment:
This description of the
BlockWithArgs
invariants is exactly what I wanted to see. Perhaps most of it should be merged into the doc comment for theBlockWithArgs::values
field above, though?
elliottt submitted PR review.
elliottt created PR review comment:
It shouldn't be called at all, thanks for catching this! This was a hold-over from when the block args were held in the same ValueList.
elliottt submitted PR review.
elliottt created PR review comment:
Happy to move it!
jameysharp submitted PR review.
jameysharp created PR review comment:
I don't feel strongly either way about using
self
imports. I just don't like having two import items with different parts of the import list like this.I'd actually be tempted to quit importing anything except
ir
andir::types
, and qualify every reference to names in their
module, given how short the module name is. But that's not something to do in this PR; it has enough else going on already.
elliottt submitted PR review.
elliottt created PR review comment:
Scratch that, it should be called from here: how else would we typecheck the branches of
brif
:)I think the issue here is that it's assumed that the args always come from the instruction. I'll rework this to take the args as a separate argument, so that it's possible to check the args for a specific block.
elliottt updated PR #5446 from trevor/brif
to main
.
jameysharp submitted PR review.
jameysharp created PR review comment:
Like with the
Conditional
case below, you can get the args fromSingleDest
's second field, can't you?I think the same is true of both
CallInfo
cases, for that matter.
jameysharp submitted PR review.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt submitted PR review.
elliottt created PR review comment:
Oh nice catch!
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt created PR review comment:
You're right, it would parse
thenblock0
. I've opted to remove those keywords for now, separating the control argument and blocks with commas instead:brif v1, block1, block2
elliottt submitted PR review.
elliottt edited PR #5446 from trevor/brif
to main
:
Add a conditional branch instruction with two targets:
brif
. This instruction will eventually replacebrz
andbrnz
, as it encompasses the behavior of both.This PR is currently rebased on top of #5450.
<!--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 #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt edited PR #5446 from trevor/brif
to main
:
Add a conditional branch instruction with two targets:
brif
. This instruction will eventually replacebrz
andbrnz
, as it encompasses the behavior of both.
<!--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 #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
branch_destination
below will need to be updated too I think to returnNone
forBrif
.
bjorn3 edited PR review comment.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
I think it does so in the order returned by
visit_branch_targets
which you implemented asthen_block
and thenelse_block
, but the code to compute this doesn't seem to be made to preserve this order, but rather preserved it by accident. See https://github.com/bytecodealliance/wasmtime/blob/94b51cdb179d08dbedd2934a3dc11e2c5aef481a/cranelift/codegen/src/machinst/blockorder.rs
bjorn3 edited PR review comment.
elliottt submitted PR review.
elliottt created PR review comment:
Yep, I have a load of local changes that rework
branch_destination
to be generated the way thatarguments
is currently, so that we can use the same trick to view multipleBlockCall
values as a slice.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt edited PR #5446 from trevor/brif
to main
:
Add a conditional branch instruction with two targets:
brif
. This instruction will eventually replacebrz
andbrnz
, as it encompasses the behavior of both.This PR also changes the
InstructionData
layout for instruction formats that holdBlockCall
values, taking the same approach we use forValue
arguments. This allowsbranch_destination
to return a slice to theBlockCall
values held in the instruction, rather than requiring that we pattern match onInstructionData
to fetch the then/else blocks.Function generation for fuzzing has been updated to generate uses of
brif
, and I've run thecranelift-fuzzgen
target locally for hours without triggering any new failures.
<!--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 has marked PR #5446 as ready for review.
elliottt requested jameysharp for a review on PR #5446.
elliottt requested cfallin for a review on PR #5446.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
Will this always be enough? I don't remember if we have any explicitly-enforced implementation limits on
br_table
successor count, but we should verify this (and leave a comment here, and possibly enforce in the validator) if we stick with au8
.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
cranelift_frontend::Switch
assumes that everything that fits in 32bits works. It is definitively realistic to have more than 256 entires in abr_table
for rust code.
elliottt submitted PR review.
elliottt created PR review comment:
Good call, I'll bump this to
u32
.
elliottt updated PR #5446 from trevor/brif
to main
.
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
Comment missing.
fitzgen created PR review comment:
I'm not seeing any test here where the
then_block
andelse_block
have different numbers of block arguments. That could be good to exercise if it isn't done elsewhere.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt updated PR #5446 from trevor/brif
to main
.
elliottt merged PR #5446.
Last updated: Dec 23 2024 at 12:05 UTC