Stream: git-wasmtime

Topic: wasmtime / issue #5464 cranelift: Rework block instructio...

view this post on Zulip Wasmtime GitHub notifications bot (Jan 07 2023 at 01:00):

elliottt commented on issue #5464:

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?

Do you mean num_fixed_value_arguments (returns 1 for a conditional)? That seems right to me. Also the wording here is slightly confusing, so I want to make sure we're on the same page: the control argument is an argument to the instruction, thus is in the instruction's argument list; the instruction just no longer has a variable-length part of the list.

This stemmed from a misunderstanding I had about what the number was indicating: I had assumed that it was indicating whether or not the number of arguments were fixed at all, which is clearly not the case. Having a single fixed argument for the conditional branches makes sense, and the change here will be that those instructions now no longer report an arg length that's greater than the number of fixed args.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 10 2023 at 01:06):

jameysharp commented on issue #5464:

This PR currently splits the branch instruction constructors into one version that accepts the same arguments as the existing constructors, and another version with an "_" suffix that takes an already-constructed BlockWithArgs. There's a fair bit of complexity in the code generator to support that split. I don't think that's bad, necessarily, but if we can find a better alternative it'd be nice.

Here are two options that I'm considering:

I think the most idiomatically-Rust way to handle this would be to introduce a new trait for type conversions which require a mutable borrow of a DataFlowGraph. Then we could arrange that the same method could be called with either an already-constructed BlockWithArgs, or with a type holding the arguments needed for constructing one, or even with other convenient alternatives like iterators.

That's particularly nice after the follow-up work of replacing brz/brnz with a two-way branch, as someone could pass a BlockWithArgs for one of the branch targets and pass the raw values for the other target. With some work, I think this approach also could generalize the value-lists mechanism that the code generator already has.

// please pick a better name
trait FromWithDataFlowGraph<T> {
    fn from(v: T, dfg: &mut DataFlowGraph) -> Self;
}

// blanket implementation for types which don't need the DFG,
// including when T == U since there's a blanket impl From<T> for T
impl<U, T: Into<U>> FromWithDataFlowGraph<T> for U {
    fn from(v: T, _dfg: &mut DataFlowGraph) -> U { v.into() }
}

// specialized implementations for a block/values pair
impl FromWithDataFlowGraph<(Block, &[Value])> for BlockWithArgs { ... }
impl<I: IntoIterator<Item = Value>> FromWithDataFlowGraph<(Block, I)> for BlockWithArgs { ... }

I imagine the main downside of that approach is that all existing uses of brz/brnz/jump would need to change to pass a pair instead of two separate arguments. So this would cause a lot of code churn: 145 calls in the wasmtime repo and however many elsewhere.

An alternative comes from observing that having an already-constructed BlockWithArgs is rare, occurring only during optimization rewrites. By my count there are five uses of the underscore-suffixed methods in this PR. Those few cases could use the underlying instruction-format constructors, which already take the raw form. For one example:

pos.func.dfg.replace(term_inst).Jump(Opcode::Jump, types::INVALID, cond_dest);

That's kind of ugly, but I think four of these five cases will go away once we replace brz/brnz with br_if. So I'm a little skeptical that any complexity in the code generator is worthwhile to make the one remaining case look pretty.

If we were starting from scratch I'd strongly recommend the first option as it minimizes special cases in the code generator and supports adding more cases like this in the future. Since we're not, what do you think of the second option?

view this post on Zulip Wasmtime GitHub notifications bot (Jan 10 2023 at 01:15):

elliottt commented on issue #5464:

If we were starting from scratch I'd strongly recommend the first option as it minimizes special cases in the code generator and supports adding more cases like this in the future. Since we're not, what do you think of the second option?

I think you're right: the second approach makes sense here given how disruptive it would be to introduce the first, and how infrequent the underscore-suffixed cases are currently.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 10 2023 at 01:16):

elliottt edited a comment on issue #5464:

If we were starting from scratch I'd strongly recommend the first option as it minimizes special cases in the code generator and supports adding more cases like this in the future. Since we're not, what do you think of the second option?

I think you're right: the second approach makes sense here given how disruptive it would be to introduce the first, and how infrequently the underscore-suffixed builders are currently used.

Last updated: Nov 22 2024 at 16:03 UTC