Stream: git-wasmtime

Topic: wasmtime / PR #5446 cranelift: Add a conditional branch i...

Wasmtime GitHub notifications bot (Dec 15 2022 at 16:31):

elliottt opened PR #5446 from trevor/brif to main:

Add a conditional branch instruction with two targets: brif. This instruction
will eventually replace brz and brnz, 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.
-->

Wasmtime GitHub notifications bot (Dec 15 2022 at 16:36):

elliottt edited PR #5446 from trevor/brif to main:

Add a conditional branch instruction with two targets: brif. This instruction will eventually replace brz and brnz, 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.
-->

Wasmtime GitHub notifications bot (Dec 15 2022 at 16:40):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 15 2022 at 17:16):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 17:16):

bjorn3 created PR review comment:

This is a gross hack. Block and Value are two entirely distinct types. They should never be mixed.

Wasmtime GitHub notifications bot (Dec 15 2022 at 17:18):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 17:18):

bjorn3 created PR review comment:

Did you mean to commit this file?

Wasmtime GitHub notifications bot (Dec 15 2022 at 17:20):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 17:20):

bjorn3 created PR review comment:

Maybe you could have a BlockOrValue type just for BlockWithArgs and then ensure that this type never leaks out of the BlockWithArgs?

Wasmtime GitHub notifications bot (Dec 15 2022 at 17:34):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 17:34):

elliottt created PR review comment:

It was pulled in after rebasing over more recent work. Thanks for catching that.

Wasmtime GitHub notifications bot (Dec 15 2022 at 17:35):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 15 2022 at 19:36):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 19:36):

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 and Value 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.

Wasmtime GitHub notifications bot (Dec 15 2022 at 20:30):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Dec 15 2022 at 20:30):

bjorn3 created PR review comment:

I agree I could have reacted much better. I will keep this in mind for the future.

Wasmtime GitHub notifications bot (Dec 15 2022 at 22:23):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:30):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 00:38):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:22):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:28):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:28):

jameysharp created PR review comment:

"bloks" -> "blocks"

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:28):

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 leave use crate::ir; as a separate item, but several times I've seen it do this kind of rewrite instead.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:28):

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?

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:28):

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 against inst_variable_args(inst), I wouldn't think that would be the right thing to do. Is it?

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:28):

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"

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:28):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:33):

elliottt created PR review comment:

What about adding a self import on line 67 and removing line 66?

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:33):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:34):

elliottt edited PR review comment.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:36):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:36):

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 the BlockWithArgs::values field above, though?

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:40):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:40):

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.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:40):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:40):

elliottt created PR review comment:

Happy to move it!

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:42):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:42):

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 and ir::types, and qualify every reference to names in the ir 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.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:51):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:51):

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.

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:57):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 02:02):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 02:02):

jameysharp created PR review comment:

Like with the Conditional case below, you can get the args from SingleDest's second field, can't you?

I think the same is true of both CallInfo cases, for that matter.

Wasmtime GitHub notifications bot (Dec 16 2022 at 02:02):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 02:08):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 02:10):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 02:11):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 02:11):

elliottt created PR review comment:

Oh nice catch!

Wasmtime GitHub notifications bot (Dec 16 2022 at 02:12):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 02:14):

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

Wasmtime GitHub notifications bot (Dec 16 2022 at 02:14):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Dec 16 2022 at 02:17):

elliottt edited PR #5446 from trevor/brif to main:

Add a conditional branch instruction with two targets: brif. This instruction will eventually replace brz and brnz, 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.
-->

Wasmtime GitHub notifications bot (Dec 16 2022 at 04:47):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 18:46):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 18:46):

elliottt edited PR #5446 from trevor/brif to main:

Add a conditional branch instruction with two targets: brif. This instruction will eventually replace brz and brnz, 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.
-->

Wasmtime GitHub notifications bot (Dec 16 2022 at 19:51):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 20:39):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 16 2022 at 22:18):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Dec 17 2022 at 00:22):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 18 2023 at 00:32):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 18 2023 at 13:54):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jan 18 2023 at 13:54):

bjorn3 created PR review comment:

branch_destination below will need to be updated too I think to return None for Brif.

Wasmtime GitHub notifications bot (Jan 18 2023 at 13:55):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Jan 18 2023 at 14:06):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jan 18 2023 at 14:06):

bjorn3 created PR review comment:

I think it does so in the order returned by visit_branch_targets which you implemented as then_block and then else_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

Wasmtime GitHub notifications bot (Jan 18 2023 at 14:07):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Jan 18 2023 at 16:29):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 18 2023 at 16:29):

elliottt created PR review comment:

Yep, I have a load of local changes that rework branch_destination to be generated the way that arguments is currently, so that we can use the same trick to view multiple BlockCall values as a slice.

Wasmtime GitHub notifications bot (Jan 18 2023 at 17:28):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 18 2023 at 18:23):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 18 2023 at 18:27):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 18 2023 at 19:23):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 18 2023 at 20:01):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 18 2023 at 20:07):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 18 2023 at 23:39):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 19 2023 at 01:05):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 19 2023 at 01:18):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 20 2023 at 22:22):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 20 2023 at 22:29):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 20 2023 at 22:49):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 21 2023 at 00:15):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 21 2023 at 21:53):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 21 2023 at 22:03):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 21 2023 at 22:12):

elliottt edited PR #5446 from trevor/brif to main:

Add a conditional branch instruction with two targets: brif. This instruction will eventually replace brz and brnz, as it encompasses the behavior of both.

This PR also changes the InstructionData layout for instruction formats that hold BlockCall values, taking the same approach we use for Value arguments. This allows branch_destination to return a slice to the BlockCall values held in the instruction, rather than requiring that we pattern match on InstructionData to fetch the then/else blocks.

Function generation for fuzzing has been updated to generate uses of brif, and I've run the cranelift-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.
-->

Wasmtime GitHub notifications bot (Jan 21 2023 at 22:12):

elliottt has marked PR #5446 as ready for review.

Wasmtime GitHub notifications bot (Jan 21 2023 at 22:12):

elliottt requested jameysharp for a review on PR #5446.

Wasmtime GitHub notifications bot (Jan 21 2023 at 22:12):

elliottt requested cfallin for a review on PR #5446.

Wasmtime GitHub notifications bot (Jan 23 2023 at 21:18):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jan 23 2023 at 21:18):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jan 23 2023 at 21:18):

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 a u8.

Wasmtime GitHub notifications bot (Jan 23 2023 at 21:25):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jan 23 2023 at 21:25):

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 a br_table for rust code.

Wasmtime GitHub notifications bot (Jan 23 2023 at 23:59):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 23 2023 at 23:59):

elliottt created PR review comment:

Good call, I'll bump this to u32.

Wasmtime GitHub notifications bot (Jan 24 2023 at 00:08):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 24 2023 at 00:09):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2023 at 00:09):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 24 2023 at 00:09):

fitzgen created PR review comment:

Comment missing.

Wasmtime GitHub notifications bot (Jan 24 2023 at 00:09):

fitzgen created PR review comment:

I'm not seeing any test here where the then_block and else_block have different numbers of block arguments. That could be good to exercise if it isn't done elsewhere.

Wasmtime GitHub notifications bot (Jan 24 2023 at 01:12):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 24 2023 at 01:18):

elliottt updated PR #5446 from trevor/brif to main.

Wasmtime GitHub notifications bot (Jan 24 2023 at 22:37):

elliottt merged PR #5446.

Last updated: Nov 22 2024 at 17:03 UTC