Stream: git-wasmtime

Topic: wasmtime / issue #5730 Remove analyze_branch and BranchInfo

Wasmtime GitHub notifications bot (Feb 07 2023 at 00:46):

elliottt commented on issue #5730:

I wonder how many of these former uses of analyze_branch could be replaced with calls to visit_block_succs? I'm pretty sure at least the ones in dominator_tree.rs and flowgraph.rs could be.

That would be a nice refactoring!

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

jameysharp commented on issue #5730:

Just so we're clear: I did push the "approve" button, with or without that hypothetical refactoring. :grin:

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

bjorn3 commented on issue #5730:

Exception handling may introduce a new instruction with semantics identical to br_table with respect to determining successors. analyze_branch would still be useful for that. In addition when a new kind of branch instruction is added, this PR causes there to no longer be a compile error or runtime error when a match is missed. Previously forgetting to add it to analyze_branch would trigger the !is_branch assertion and if a new BranchInfo variant was added you did get a compile error.

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

bjorn3 commented on issue #5730:

Maybe the visit_block_succs refactoring could be done and the remaining cases could get the !is_branch assertion added back?

Wasmtime GitHub notifications bot (Feb 07 2023 at 16:35):

elliottt commented on issue #5730:

Exception handling may introduce a new instruction with semantics identical to br_table with respect to determining successors. analyze_branch would still be useful for that.

Would that have turned into a return value of BranchInfo::Table in analyze_branch? If so, you would still have needed to re-match the opcode to apply special behavior for your new instruction.

In addition when a new kind of branch instruction is added, this PR causes there to no longer be a compile error or runtime error when a match is missed. Previously forgetting to add it to analyze_branch would trigger the !is_branch assertion and if a new BranchInfo variant was added you did get a compile error.

This is true, however my hope was that we've now arrived at the necessary set of branch instructions and won't need to add any others. We have unconditional jump, conditional branch, and branch through table instructions, which feel like the right building blocks for more complicated control flow. I think the switch module in cranelift-frontend is a great example of this.

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

bjorn3 commented on issue #5730:

Would that have turned into a return value of BranchInfo::Table in analyze_branch?

Yes

If so, you would still have needed to re-match the opcode to apply special behavior for your new instruction.

Yeah, in some cases that is indeed necessary. In other cases it doesn't actually care about which exact branch instruction is used. Just which successors exist.

This is true, however my hope was that we've now arrived at the necessary set of branch instructions and won't need to add any others. We have unconditional jump, conditional branch, and branch through table instructions, which feel like the right building blocks for more complicated control flow. I think the switch module in cranelift-frontend is a great example of this.

I see. For exceptions I am pretty sure we will need an invoke/invoke_indirect instruction to be able to define the unwinding path that a call will take. From the clif ir perspective this will be a branch instruction which branches when an exception happens. I don't think this is implementable without support from cranelift-codegen.

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

cfallin commented on issue #5730:

+1 to invoke-family instructions: fundamentally we can't represent the zero-cost exception model without it, I think, because the unwinder is external and effectively jumps to a different return address.

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

cfallin edited a comment on issue #5730:

+1 to invoke-family instructions: fundamentally we can't represent the zero-cost exception model without it (EDIT: being another kind of branch), I think, because the unwinder is external and effectively jumps to a different return address.

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

elliottt commented on issue #5730:

I see. For exceptions I am pretty sure we will need an invoke/invoke_indirect instruction to be able to define the unwinding path that a call will take. From the clif ir perspective this will be a branch instruction which branches when an exception happens. I don't think this is implementable without support from cranelift-codegen.

I'll make a PR that both implements @jameysharp's earlier refactoring suggestion, and add assert!(!inst.is_branch()) to all the cases that I modified when removing BranchInfo. Does that seem sufficient for not making the addition of invoke/invoke_indirect not too painful?

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

bjorn3 commented on issue #5730:

I think that is sufficient.

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

elliottt edited a comment on issue #5730:

I see. For exceptions I am pretty sure we will need an invoke/invoke_indirect instruction to be able to define the unwinding path that a call will take. From the clif ir perspective this will be a branch instruction which branches when an exception happens. I don't think this is implementable without support from cranelift-codegen.

I'll make a PR that both implements @jameysharp's earlier refactoring suggestion, and add assert!(!inst.is_branch()) to all the cases that I modified when removing BranchInfo. Does that seem sufficient for not making the addition of invoke/invoke_indirect too painful?

(edited to remove accidental double negative)

Last updated: Nov 22 2024 at 17:03 UTC