Stream: git-wasmtime

Topic: wasmtime / PR #5731 Rework br_table to use BlockCall

Wasmtime GitHub notifications bot (Feb 07 2023 at 01:10):

elliottt opened PR #5731 from trevor/br-table-block-args to main:

Rework br_table to use BlockCall, allowing us to avoid adding new nodes during ssa construction to hold block arguments.

<!--

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 (Feb 07 2023 at 01:11):

elliottt edited PR #5731 from trevor/br-table-block-args to main:

Rework br_table to use BlockCall, allowing us to avoid adding new nodes during ssa construction to hold block arguments.

• [ ] Figure out how to adapt branch_destination and branch_destination_mut

<!--

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 (Feb 07 2023 at 01:17):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 07 2023 at 11:39):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 07 2023 at 11:39):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 07 2023 at 11:39):

bjorn3 created PR review comment:

Is there some limitation in the code generator that requires this change?

Wasmtime GitHub notifications bot (Feb 07 2023 at 18:25):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 07 2023 at 18:25):

elliottt created PR review comment:

Yes, with the introduction of BlockCall, block arguments got floated to the end of instruction builders. I think that's fixable, but in the interest of getting something building I switched it locally. I'll add a TODO to investigate fixing this.

Wasmtime GitHub notifications bot (Feb 07 2023 at 18:25):

elliottt edited PR #5731 from trevor/br-table-block-args to main:

Rework br_table to use BlockCall, allowing us to avoid adding new nodes during ssa construction to hold block arguments.

• [ ] Figure out how to adapt branch_destination and branch_destination_mut
• [ ] Determine if it's possible to keep the existing br_table builder argument order

<!--

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 (Feb 07 2023 at 21:31):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 08 2023 at 05:33):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 08 2023 at 06:53):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 08 2023 at 11:26):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 08 2023 at 11:26):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 08 2023 at 11:26):

bjorn3 created PR review comment:

This code may need to be preserved for invoke depending on how it will be implemented. The exception path gets some block arguments from the personality function, so either those would be added to the block arguments listed in BlockCall of the invoke instruction, or the invoke instruction would only list a Block and synthesize the full block argument list internally. What do you think would be cleaner?

Wasmtime GitHub notifications bot (Feb 08 2023 at 16:28):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 08 2023 at 16:28):

elliottt created PR review comment:

I like the idea of using a BlockCall on the invoke instruction. I think the path for doing that should be pretty well worn after adding brif, but if you run into any problems using BlockCall I'd be happy to help work through them :)

Wasmtime GitHub notifications bot (Feb 08 2023 at 16:42):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 08 2023 at 16:42):

bjorn3 created PR review comment:

The difference between brif and invoke is that brif passes all block arguments in the instruction to the destination block, but for invoke there would be some extra block arguments not specified in the BlockCall. Do you add those at the start or the end? And is there any code that expects there to be no block arguments added? The following is pseudocode to demonstrate the issue:

block0:
    v1 = iconst.i64 42
    invoke fn0(), block1, block2(v1)

block1:
    trap user0

block2(v2: i64, v3: i64):
    ; this is the landingpad
    ; either v2 or v3 is the extra argument containing a value passed by the personality function. which one should be the extra argument?
    trap user0

Wasmtime GitHub notifications bot (Feb 08 2023 at 16:44):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 08 2023 at 16:44):

elliottt created PR review comment:

Ah, interesting! I would add them to the end and still use a BlockCall. It's sort of like a partially applied function that way.

Wasmtime GitHub notifications bot (Feb 08 2023 at 16:51):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 08 2023 at 16:51):

bjorn3 created PR review comment:

Sure, will try that once I get to adding the invoke instruction.

Wasmtime GitHub notifications bot (Feb 08 2023 at 21:50):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 08 2023 at 22:27):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 09 2023 at 00:31):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 09 2023 at 00:31):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 09 2023 at 01:27):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 09 2023 at 01:34):

elliottt edited PR #5731 from trevor/br-table-block-args to main:

Rework br_table to use BlockCall, allowing us to avoid adding new nodes during ssa construction to hold block arguments.

• [x] Figure out how to adapt branch_destination and branch_destination_mut
• [ ] Determine if it's possible to keep the existing br_table builder argument order

<!--

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 (Feb 09 2023 at 17:36):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 09 2023 at 17:55):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 09 2023 at 19:49):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 10 2023 at 00:37):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 10 2023 at 03:00):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 10 2023 at 17:45):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 10 2023 at 17:46):

elliottt edited PR #5731 from trevor/br-table-block-args to main:

Rework br_table to use BlockCall, allowing us to avoid adding new nodes during ssa construction to hold block arguments.

• [x] Figure out how to adapt branch_destination and branch_destination_mut
• [x] Determine if it's possible to keep the existing br_table builder argument order

<!--

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 (Feb 10 2023 at 18:03):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 10 2023 at 18:04):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 10 2023 at 18:05):

elliottt edited PR #5731 from trevor/br-table-block-args to main:

Rework br_table to use BlockCall, allowing us to avoid adding new nodes during ssa construction to hold block arguments.

<!--

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 (Feb 13 2023 at 18:51):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 13 2023 at 18:51):

elliottt created PR review comment:

This change is due to the layout of JumpTableData, which keeps the default BlockCall last. The reason for mirroring the order here is so that in machinst::lower we can traverse the slice of BlockCall values directly when building up targets for branch lowering.

Wasmtime GitHub notifications bot (Feb 13 2023 at 18:52):

elliottt edited PR review comment.

Wasmtime GitHub notifications bot (Feb 13 2023 at 18:58):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 13 2023 at 18:58):

elliottt created PR review comment:

This is due to the change in traversal order of the jump table: traversing the default label last means that it is emitted last.

Wasmtime GitHub notifications bot (Feb 13 2023 at 19:08):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 13 2023 at 20:11):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 13 2023 at 21:52):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 13 2023 at 22:20):

elliottt edited PR #5731 from trevor/br-table-block-args to main:

Rework br_table to use BlockCall, allowing us to avoid adding new nodes during ssa construction to hold block arguments. Additionally, many places where we previously matched on InstructionData to extract branch destinations can be replaced with a use of branch_destination or branch_destination_mut.

<!--

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 (Feb 14 2023 at 01:03):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 14 2023 at 01:07):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:02):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:02):

elliottt created PR review comment:

The changes in this file aren't necessary for this PR, but did show up as necessary when making changes to the old form of br_table.

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:42):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:42):

elliottt created PR review comment:

This isn't necessary for the br_table changes, but it seemed nice to avoid the slice creation.

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:43):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:43):

elliottt has marked PR #5731 as ready for review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:43):

elliottt requested jameysharp for a review on PR #5731.

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:47):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:47):

bjorn3 created PR review comment:

Maybe also add a test using block arguments?

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:52):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:52):

elliottt created PR review comment:

I've added both runtests and x64 compile tests for uses of br_table with block arguments. My feeling was that those would be sufficient as this PR stops splitting critical edges in ssa construction, was there an additional case you were thinking of here?

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:56):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:56):

bjorn3 created PR review comment:

I guess that works too. I was thinking that we have a test of creating jump tables here, so we might as well add the new case of having block arguments here.

Wasmtime GitHub notifications bot (Feb 14 2023 at 17:56):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 14 2023 at 18:00):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2023 at 18:00):

elliottt created PR review comment:

I'll add different arguments to the two uses of e1, and assert that they're the same.

Wasmtime GitHub notifications bot (Feb 14 2023 at 18:00):

elliottt edited PR review comment.

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

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:45):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:45):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:45):

jameysharp created PR review comment:

After reading this comment, I was left wondering why this whole function can't just be replaced with a loop over branch_destination. I forgot the boolean argument to the closure depends on these details. Could you add that detail to the comment, for the next time I forget how everything works?

Of course, I think all but one caller of this function ignore the boolean argument, and with this PR, all except that one could be replaced with a small loop over branch_destination. Maybe we should look at whether the call in cranelift/codegen/src/machinst/blockorder.rs really needs to know this distinction, and see if we can eliminate this function entirely now...

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:45):

jameysharp created PR review comment:

This appears to have been the only use of arrayvec in this entire git repository. Let's remove it.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:45):

jameysharp created PR review comment:

For slices, iter and into_iter are identical. So I'd use the shorter version here. Bonus points if that gets rustfmt to put this back on one line, though I'm not betting on that.

                .iter()

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:45):

jameysharp created PR review comment:

Can rewrite_branch_destination become a small loop over branch_destination_mut now every case uses BlockCall?

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:45):

jameysharp created PR review comment:

I think it's a good assertion to add regardless, so I'm glad you kept this in.

Wasmtime GitHub notifications bot (Feb 15 2023 at 02:45):

jameysharp created PR review comment:

This isn't sufficient to generate br_table with block params. We also need to update the match on BlockTerminatorKind::BrTable elsewhere to use generate_target_block instead of selecting from forward_blocks_without_params. Also that should be the last use of the latter as well as a variety of other machinery which can all be removed now. Hooray!

Wasmtime GitHub notifications bot (Feb 15 2023 at 12:39):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 12:39):

afonso360 created PR review comment:

Alongside what @jameysharp is saying above, we can now select Switch and BrTable more often, since they no longer require a special block without params.

Could you also update this: https://github.com/bytecodealliance/wasmtime/blob/e10094dcd6d0354628255a6f2e69c1e4c327d6e7/cranelift/fuzzgen/src/function_generator.rs#L1806-L1815 So that we allow BrTable and Switch alongside Jump and Br?

The only other places where we use forward_blocks_without_params, is when selecting blocks for Switch targets, and those can also now carry params, so I agree, lets remove it!

Wasmtime GitHub notifications bot (Feb 15 2023 at 12:44):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Feb 15 2023 at 17:48):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 17:48):

elliottt created PR review comment:

Yep, good call!

Wasmtime GitHub notifications bot (Feb 15 2023 at 17:50):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 17:50):

elliottt created PR review comment:

It uses it to record those branch targets as indirect. We might be able to do something gross to recover that information some other way, but I feel like it's fine to duplicate the matching logic here. I'll add the additional comment you mentioned :)

Wasmtime GitHub notifications bot (Feb 15 2023 at 17:54):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 17:54):

elliottt created PR review comment:

Unfortunately that didn't bring us back to a single line :(

Wasmtime GitHub notifications bot (Feb 15 2023 at 18:09):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 15 2023 at 18:09):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 18:09):

elliottt created PR review comment:

I believe I've made the changes you both requested, could you take a quick look and make sure?

Wasmtime GitHub notifications bot (Feb 15 2023 at 18:15):

elliottt requested jameysharp for a review on PR #5731.

Wasmtime GitHub notifications bot (Feb 15 2023 at 18:15):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 18:15):

afonso360 created PR review comment:

LGTM! :+1:

Wasmtime GitHub notifications bot (Feb 15 2023 at 18:27):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 18:27):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 18:27):

jameysharp created PR review comment:

I have a mild preference for the iterator chain approach here, mostly to avoid thinking about allocation capacity. A neat Rust trick is that Result implements FromIterator: if any element of the iterator is Err then that's returned, otherwise all the Ok values are collected into the underlying collection type. So I believe this should work:

                        let targets = (0..target_count).map(|_| self.generate_target_block(block)).collect()?;

Wasmtime GitHub notifications bot (Feb 15 2023 at 19:19):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2023 at 19:19):

elliottt created PR review comment:

Very nice! I started with the iterator chain and went back to a for loop when I couldn't think of an elegant way to handle the Result. I'll give this a try :)

Wasmtime GitHub notifications bot (Feb 15 2023 at 19:27):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 15 2023 at 19:52):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 16 2023 at 00:42):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 16 2023 at 01:32):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Feb 16 2023 at 01:32):

elliottt created PR review comment:

This is a bit unfortunate: the table throws off capstone, which causes it to not see the movl $4, %edx and the label for offset 0x39. I have a fix in #5798 that I've confirmed locally outputs the mov and label.

Wasmtime GitHub notifications bot (Feb 16 2023 at 04:00):

elliottt updated PR #5731 from trevor/br-table-block-args to main.

Wasmtime GitHub notifications bot (Feb 16 2023 at 17:23):

elliottt merged PR #5731.

Last updated: Dec 23 2024 at 12:05 UTC