wasmtime / PR #5464 cranelift: Rework block instructions ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #5464 cranelift: Rework block instructions ...

Wasmtime GitHub notifications bot (Dec 17 2022 at 01:49):

elliottt opened PR #5464 from trevor/block-with-args to main:

Add a new type BlockWithArgs that represents the pair of a block name with arguments to be passed to it. Rework the implementation of jump, brz, and brnz to use BlockWithArgs instead of storing the block arguments as varargs in the instruction's ValueList.

Wasmtime GitHub notifications bot (Dec 17 2022 at 01:54):

elliottt edited PR #5464 from trevor/block-with-args to main:

Add a new type BlockWithArgs that represents the pair of a block name with arguments to be passed to it. Rework the implementation of jump, brz, and brnz to use BlockWithArgs instead of storing the block arguments as varargs in the instruction's ValueList.

Some open questions:

[ ] Does the return value of num_fixed_actuals function still make sense for brz and brnz even though they no longer store their control argument in the instruction's argument list?

[ ]The reworked builder api for jump, brz, and brnz is not very nice to work with. Can we generate two variations of each function, one that takes a BlockWithArgs and one that takes the pair of Block and Value slice?

[ ] I'm not 100% confident that I've found everywhere that we're using instruction arguments and block arguments interchangeably. Is there a good way to track this down beyond fuzzing?

Wasmtime GitHub notifications bot (Dec 17 2022 at 01:54):

elliottt edited PR #5464 from trevor/block-with-args to main:

Add a new type BlockWithArgs that represents the pair of a block name with arguments to be passed to it. Rework the implementation of jump, brz, and brnz to use BlockWithArgs instead of storing the block arguments as varargs in the instruction's ValueList.

Some open questions:

[ ] Does the return value of num_fixed_actuals function still make sense for brz and brnz even though they no longer store their control argument in the instruction's argument list?

[ ] The reworked builder api for jump, brz, and brnz is not very nice to work with. Can we generate two variations of each function, one that takes a BlockWithArgs and one that takes the pair of Block and Value slice?

[ ] I'm not 100% confident that I've found everywhere that we're using instruction arguments and block arguments interchangeably. Is there a good way to track this down beyond fuzzing?

Wasmtime GitHub notifications bot (Dec 19 2022 at 17:46):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Dec 19 2022 at 18:24):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 19 2022 at 18:24):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 19 2022 at 18:24):

cfallin created PR review comment:

Can we note that args are included here (in contrast to label below), and possibly also call it block_with_args throughout for clarity?

Wasmtime GitHub notifications bot (Dec 19 2022 at 18:24):

cfallin created PR review comment:

Likewise here, note that args are included

Wasmtime GitHub notifications bot (Dec 19 2022 at 18:24):

cfallin created PR review comment:

Is this correct for conditional branches? We need to skip the first arg from the elab stack, no?

Wasmtime GitHub notifications bot (Dec 19 2022 at 18:24):

cfallin created PR review comment:

The code duplication here is pretty unfortunate. Could we wrap it in a closure (handle_use or somesuch)?

(Making it a closure will probably require closure args for borrows so the closure itself doesn't hold the borrows for too long)

Wasmtime GitHub notifications bot (Dec 19 2022 at 18:24):

cfallin created PR review comment:

Somewhat confusing to name this local cell if it's not actually a Cell or some other kind of interior mutability -- can we just call it first_elem or something?

Wasmtime GitHub notifications bot (Dec 19 2022 at 18:24):

cfallin created PR review comment:

We can probably preallocate the list with the appropriate length upfront, since args is a slice and we know its length?

Wasmtime GitHub notifications bot (Dec 19 2022 at 18:25):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Dec 19 2022 at 18:34):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 19 2022 at 18:34):

jameysharp created PR review comment:

Huh, apparently EntityList doesn't have a with_capacity constructor. Its extend method does make some use of Iterator::size_hint though, so this reallocs the list at most once, when it has only one element in it. We could use Iterator::chain to preallocate the right size but I'm not convinced it's worth bothering.

Wasmtime GitHub notifications bot (Dec 19 2022 at 18:55):

elliottt updated PR #5464 from trevor/block-with-args to main.

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

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 06 2023 at 01:45):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 06 2023 at 01:46):

elliottt edited PR #5464 from trevor/block-with-args to main:

Add a new type BlockWithArgs that represents the pair of a block name with arguments to be passed to it. Rework the implementation of jump, brz, and brnz to use BlockWithArgs instead of storing the block arguments as varargs in the instruction's ValueList.

Some open questions:

[ ] Does the return value of num_fixed_actuals function still make sense for brz and brnz even though they no longer store their control argument in the instruction's argument list?

[x] The reworked builder api for jump, brz, and brnz is not very nice to work with. Can we generate two variations of each function, one that takes a BlockWithArgs and one that takes the pair of Block and Value slice?

[ ] I'm not 100% confident that I've found everywhere that we're using instruction arguments and block arguments interchangeably. Is there a good way to track this down beyond fuzzing?

Wasmtime GitHub notifications bot (Jan 07 2023 at 00:50):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 07 2023 at 00:50):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 07 2023 at 00:51):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 07 2023 at 00:51):

elliottt created PR review comment:

Yep, this wasn't correct. Switching to using a map function for values made it much easier to get this right.

Wasmtime GitHub notifications bot (Jan 07 2023 at 00:52):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 07 2023 at 00:52):

elliottt created PR review comment:

I've added some functions to abstract over the values of an instruction to avoid this sort of duplication, and am in the process of auditing uses of the existing arg accessors to determine if they need to be changed.

Wasmtime GitHub notifications bot (Jan 07 2023 at 01:00):

elliottt edited PR #5464 from trevor/block-with-args to main:

Add a new type BlockWithArgs that represents the pair of a block name with arguments to be passed to it. Rework the implementation of jump, brz, and brnz to use BlockWithArgs instead of storing the block arguments as varargs in the instruction's ValueList.

Some open questions:

[x] Does the return value of num_fixed_actuals function still make sense for brz and brnz even though they no longer store their control argument in the instruction's argument list?

[x] The reworked builder api for jump, brz, and brnz is not very nice to work with. Can we generate two variations of each function, one that takes a BlockWithArgs and one that takes the pair of Block and Value slice?

[ ] I'm not 100% confident that I've found everywhere that we're using instruction arguments and block arguments interchangeably. Is there a good way to track this down beyond fuzzing?

Wasmtime GitHub notifications bot (Jan 07 2023 at 02:00):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 07 2023 at 02:03):

elliottt updated PR #5464 from trevor/block-with-args to main.

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

elliottt updated PR #5464 from trevor/block-with-args to main.

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

elliottt updated PR #5464 from trevor/block-with-args to main.

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

elliottt updated PR #5464 from trevor/block-with-args to main.

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

elliottt edited PR #5464 from trevor/block-with-args to main:

Add a new type BlockWithArgs that represents the pair of a block name with arguments to be passed to it. Rework the implementation of jump, brz, and brnz to use BlockWithArgs instead of storing the block arguments as varargs in the instruction's ValueList.

Some open questions:

[x] Does the return value of num_fixed_actuals function still make sense for brz and brnz even though they no longer store their control argument in the instruction's argument list?

[x] The reworked builder api for jump, brz, and brnz is not very nice to work with. Can we generate two variations of each function, one that takes a BlockWithArgs and one that takes the pair of Block and Value slice?

[x] I'm not 100% confident that I've found everywhere that we're using instruction arguments and block arguments interchangeably. Is there a good way to track this down beyond fuzzing?

Wasmtime GitHub notifications bot (Jan 10 2023 at 00:02):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 10 2023 at 00:02):

jameysharp created PR review comment:

I was concerned about silently deleting the "Can't use .sum() for Cost types" comment and then effectively implementing our own version of Iterator::sum. But I see that Cost does implement std::ops::Add and this statement was already relying on + being overloaded. So I'm pretty sure this change is correct.

Wasmtime GitHub notifications bot (Jan 10 2023 at 00:02):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 10 2023 at 01:30):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 10 2023 at 16:48):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 10 2023 at 16:50):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 10 2023 at 16:54):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 10 2023 at 19:08):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 10 2023 at 19:30):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 10 2023 at 19:31):

elliottt edited PR #5464 from trevor/block-with-args to main:

Add a new type BlockWithArgs that represents the pair of a block name with arguments to be passed to it. Rework the implementation of jump, brz, and brnz to use BlockWithArgs instead of storing the block arguments as varargs in the instruction's ValueList.

Some open questions:

[x] Does the return value of num_fixed_actuals function still make sense for brz and brnz even though they no longer store their control argument in the instruction's argument list?

[x] The reworked builder api for jump, brz, and brnz is not very nice to work with. Can we generate two variations of each function, one that takes a BlockWithArgs and one that takes the pair of Block and Value slice?

[x] I'm not 100% confident that I've found everywhere that we're using instruction arguments and block arguments interchangeably. Is there a good way to track this down beyond fuzzing?

Wasmtime GitHub notifications bot (Jan 10 2023 at 19:35):

elliottt edited PR #5464 from trevor/block-with-args to main:

Add a new type BlockCall that represents the pair of a block name with arguments to be passed to it. (The mnemonic here is that it looks a bit like a function call.) Rework the implementation of jump, brz, and brnz to use BlockCall instead of storing the block arguments as varargs in the instruction's ValueList.

To ensure that we're processing value references from BlockCall values held in instructions, introduce three new functions on DataFlowGraph: inst_values, inst_values_mut, and inst_values_mut_with. The first function returns an iterator that traverses both the instruction and block arguments, while the latter two return a custom type that has a single map function for mutating the values referenced. The inst_values_mut_with variant exists for cases where the DataFlowGraph being mutated is held within another structure that needs to be mutated simultaneously.

Wasmtime GitHub notifications bot (Jan 10 2023 at 19:36):

elliottt has marked PR #5464 as ready for review.

Wasmtime GitHub notifications bot (Jan 10 2023 at 19:39):

elliottt edited PR #5464 from trevor/block-with-args to main:

Add a new type BlockCall that represents the pair of a block name with arguments to be passed to it. (The mnemonic here is that it looks a bit like a function call.) Rework the implementation of jump, brz, and brnz to use BlockCall instead of storing the block arguments as varargs in the instruction's ValueList.

To ensure that we're processing value references from BlockCall values held in instructions, introduce three new functions on DataFlowGraph: inst_values, inst_values_mut, and inst_values_mut_with. The first function returns an iterator that traverses both the instruction and block arguments, while the latter two return a custom type that has a single map function for mutating the values referenced. The inst_values_mut_with variant exists for cases where the DataFlowGraph being mutated is held within another structure that needs to be mutated simultaneously.

I went through the existing uses of inst_args, inst_fixed_args, inst_variable_args, and their mut variants to decide whether or not they should be using the new iterator interface introduced above. Many needed to be migrated over, but there were cases where the original instruction was correct: the fuzzgen passes for fcvt and int_divz are examples of where the block args were never being indexed.

Wasmtime GitHub notifications bot (Jan 10 2023 at 19:39):

elliottt requested jameysharp for a review on PR #5464.

Wasmtime GitHub notifications bot (Jan 10 2023 at 23:01):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 10 2023 at 23:29):

elliottt edited PR #5464 from trevor/block-with-args to main:

Add a new type BlockCall that represents the pair of a block name with arguments to be passed to it. (The mnemonic here is that it looks a bit like a function call.) Rework the implementation of jump, brz, and brnz to use BlockCall instead of storing the block arguments as varargs in the instruction's ValueList.

To ensure that we're processing block arguments from BlockCall values in instructions, three new functions have been introduced on DataFlowGraph that both sets of arguments:

inst_values - returns an iterator that traverses values in the instruction and block arguments

map_inst_values - applies a function to each value in the instruction and block arguments

overwrite_inst_values - overwrite all values in an instruction and block arguments with values from the iterator

I went through the existing uses of inst_args, inst_fixed_args, inst_variable_args, and their mut variants to decide whether or not they should be using the new iterator interface introduced above. Many needed to be migrated over, but there were cases where the original instruction was correct: the fuzzgen passes for fcvt and int_divz are examples of where the block args were never being indexed.

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

I believe every part of this iterator supports access from both ends. If true, that means you can change the declaration this way, I think, and then callers can use Iterator::rev if they want.
    pub fn inst_values<'dfg>(&'dfg self, inst: Inst) -> impl DoubleEndedIterator<Item = Value> + 'dfg {

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

Continuing the theme, how about renaming block here to block_call and extending the documentation to "Destination basic block, with its arguments provided"?

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

I guess this blank line doesn't really need to be added here.

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

Since it turned out that inst_values can return an iterator, you can replace all the uses of for_each with for loops. In cases like this one that's good both because it minimizes the diff here and because for_each is generally discouraged, as a Rust idiom. Switching back to for loops is a bigger win for the uses of try_for_each, which can just use break statements or the ? operator instead of ControlFlow.

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

The comment isn't quite true: This vector can't be moved onto self, since the reason for introducing it is to avoid holding borrows of self across calls to self.elaborate_eclass_use.

But this function, elaborate_block, is only called from elaborate_domtree, so the vector could be allocated there and passed in by reference. That function in turn is called only once for the pass, so allocating the temporary storage there is as good as storing it on self would be.

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

I think most of the changes around here can be undone, assuming that self.func.dfg.inst_values(inst) can return a DoubleEndedIterator.

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

I've never had reason to use this Rust format-string syntax before, but this seems like a good place for it:
                "let {0} = self.data_flow_graph_mut().block_with_args({0}_label, {0}_args);",
                op.name

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

I'm amused that leaving the semicolon off here is legal, but let's not...
                *arg = new_arg.value;

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

A minor cleanup here is to call Vec::drain instead of iter, copied, and clear.
            self.func
                .dfg
                .overwrite_inst_values(inst, elab_values.drain(..));

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

Minor style note: a for loop always calls into_iter, so you don't need to do it here.
        for mut block in self.insts[inst].branch_destination() {

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

Like the for_each calls, now that inst_values is an iterator this can turn back into a for loop with an early return.

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

I don't see that this count_values method is used anywhere. Am I missing something?

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

Since map_inst_values writes back unconditionally, let's not bother checking for unchanged values here either. (I expect rustfmt will want the outer curly braces removed too, but GitHub won't let me do suggestions on the full range of lines here.)
            resolve_aliases(&dfg.values, arg)

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

into_iter isn't necessary here either:
        for mut block in self.insts[inst].branch_destination() {

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

Good call updating this comment. Maybe "a count only of the Value arguments" or something along those lines? It doesn't include various kinds of immediate arguments either.

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

This closure is called with block set to branch(), which is then shadowed here with the same value. I think this closure shouldn't take any arguments.

With that change, I think this will be the only call to branch(), so I'd inline it into continue_at.

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

Do you suppose this function should be renamed to block_call now, or perhaps make_block_call or something? I think calling it block_with_args isn't terrible but it's worth taking a moment to think about it.

Also, there are various comments that still refer to BlockWithArgs, like this one and a bunch of places in instructions.rs.

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

I wonder if this alternative is any better (after applying rustfmt, anyway). What do you think?
            let branch_args = self.f.dfg.insts[inst].branch_destination().into_iter().flat_map(|block| block.args_slice(&self.f.dfg.value_lists));

Wasmtime GitHub notifications bot (Jan 11 2023 at 02:27):

jameysharp created PR review comment:

Another place where into_iter isn't necessary:

            for branch in dfg.insts[pred.inst].branch_destination_mut() {

Wasmtime GitHub notifications bot (Jan 11 2023 at 04:47):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 11 2023 at 04:47):

elliottt created PR review comment:

I'm surprised this wasn't caught by rustfmt!

Wasmtime GitHub notifications bot (Jan 11 2023 at 04:47):

elliottt deleted PR review comment.

Wasmtime GitHub notifications bot (Jan 11 2023 at 04:48):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 11 2023 at 04:48):

elliottt created PR review comment:

It was left-over from an earlier refactoring, thanks for catching it!

Wasmtime GitHub notifications bot (Jan 11 2023 at 04:50):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 11 2023 at 04:50):

elliottt created PR review comment:

Ah, this was to avoid a warning. Do you mind if we leave these for now and remove them once branch_destination returns a slice?

Wasmtime GitHub notifications bot (Jan 11 2023 at 04:56):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 11 2023 at 16:09):

jameysharp created PR review comment:

Oh, that makes sense! Sure, leave the .into_iter() calls in, that's fine.

Wasmtime GitHub notifications bot (Jan 11 2023 at 16:09):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 11 2023 at 16:21):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 11 2023 at 16:21):

jameysharp created PR review comment:

I guess the current CI failure is that this use item is no longer necessary.

Wasmtime GitHub notifications bot (Jan 11 2023 at 17:06):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 11 2023 at 17:06):

elliottt created PR review comment:

I've removed the comment and implemented that change, thanks!

Wasmtime GitHub notifications bot (Jan 11 2023 at 17:43):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 11 2023 at 17:43):

elliottt created PR review comment:

I like it!

Wasmtime GitHub notifications bot (Jan 11 2023 at 19:42):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 11 2023 at 19:50):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 11 2023 at 19:51):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 11 2023 at 22:01):

elliottt edited PR #5464 from trevor/block-with-args to main:

Add a new type BlockCall that represents the pair of a block name with arguments to be passed to it. (The mnemonic here is that it looks a bit like a function call.) Rework the implementation of jump, brz, and brnz to use BlockCall instead of storing the block arguments as varargs in the instruction's ValueList.

To ensure that we're processing block arguments from BlockCall values in instructions, three new functions have been introduced on DataFlowGraph that both sets of arguments:

inst_values - returns an iterator that traverses values in the instruction and block arguments

map_inst_values - applies a function to each value in the instruction and block arguments

overwrite_inst_values - overwrite all values in an instruction and block arguments with values from the iterator

I went through the existing uses of inst_args, inst_fixed_args, inst_variable_args, and their mut variants to decide whether or not they should be using the new iterator interface introduced above. Many needed to be migrated over, but there were cases where the original instruction was correct: the fuzzgen passes for fcvt and int_divz are examples of where the block args were never being indexed.

Fixes #5451

Wasmtime GitHub notifications bot (Jan 11 2023 at 23:06):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 11 2023 at 23:32):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 12 2023 at 00:49):

elliottt edited PR #5464 from trevor/block-with-args to main:

Add a new type BlockCall that represents the pair of a block name with arguments to be passed to it. (The mnemonic here is that it looks a bit like a function call.) Rework the implementation of jump, brz, and brnz to use BlockCall instead of storing the block arguments as varargs in the instruction's ValueList.

To ensure that we're processing block arguments from BlockCall values in instructions, three new functions have been introduced on DataFlowGraph that both sets of arguments:

inst_values - returns an iterator that traverses values in the instruction and block arguments

map_inst_values - applies a function to each value in the instruction and block arguments

overwrite_inst_values - overwrite all values in an instruction and block arguments with values from the iterator

I went through the existing uses of inst_args, inst_fixed_args, inst_variable_args, and their mut variants to decide whether or not they should be using the new iterator interface introduced above. Many needed to be migrated over, but there were cases where the original instruction was correct: the fuzzgen passes for fcvt and int_divz are examples of where the block args were never being indexed.

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

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 12 2023 at 16:27):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 13 2023 at 23:50):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 13 2023 at 23:50):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 13 2023 at 23:50):

jameysharp created PR review comment:

Should this just use inst_values, now that that's an iterator? I'm not sure I quite understand how the stack is being used here, but I don't _think_ it requires these to be in separate iterators, so I think this simplification should be okay.
                stack.push(f.dfg.inst_values(src_inst));

Wasmtime GitHub notifications bot (Jan 13 2023 at 23:50):

jameysharp created PR review comment:

Would you mind passing the block from the caller? It's already done the necessary pattern match and can avoid the .unwrap().

Wasmtime GitHub notifications bot (Jan 13 2023 at 23:50):

jameysharp created PR review comment:

Most (maybe all?) of this complexity should go away when the br_if migration is done, so I don't care whether you make this change before merging, but:

I just noticed we can delete the BranchOrderKind enum and deduplicate the replace calls below by just directly returning which opcode the Branch should use after this rewrite:
                        Opcode::Brz => Opcode::Brnz,
                        Opcode::Brnz => Opcode::Brz,
We could also get rid of some noise by moving the replace calls inside the inner match, instead of returning a tuple below and having to let-bind it above.

Wasmtime GitHub notifications bot (Jan 17 2023 at 16:39):

elliottt created PR review comment:

Yep, it definitely should, thanks for catching that one!

Wasmtime GitHub notifications bot (Jan 17 2023 at 16:39):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 17:01):

elliottt created PR review comment:

I remember what happened when I tried this with a previous version of this branch: we need to change the element type of stack to Box<dyn Iterator<Item=Value>> for that change to work because inst_values returns an impl Iterator<Item=Value>. Will the heap indirection be worth the change here?

Wasmtime GitHub notifications bot (Jan 17 2023 at 17:01):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 17:11):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 17 2023 at 21:07):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 21:07):

elliottt created PR review comment:

We went back and forth on alternate approaches here in a DM, and @jameysharp found a great one that he's going to push in a separate PR.

Wasmtime GitHub notifications bot (Jan 17 2023 at 22:01):

elliottt submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 22:01):

elliottt created PR review comment:

This refactoring was made in #5586.

Wasmtime GitHub notifications bot (Jan 17 2023 at 22:44):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 17 2023 at 22:51):

elliottt updated PR #5464 from trevor/block-with-args to main.

Wasmtime GitHub notifications bot (Jan 17 2023 at 22:52):

elliottt updated PR #5464 from trevor/block-with-args to main.

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

elliottt merged PR #5464.

Last updated: Dec 23 2024 at 13:07 UTC