Rework br_table to use BlockCall by elliottt · Pull Request #5731 · bytecodealliance/wasmtime

elliottt · 2023-02-07T01:10:36Z

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.

bjorn3

This is a nice simplification. I'm curious how much of a (negative or positive) perf impact this will have though.

cranelift/codegen/meta/src/shared/instructions.rs

Move the storage for jump tables off of FunctionStencil and onto DataFlowGraph. This change is in service of #5731, making it easier to access the jump table data in the context of helpers like inst_values.

bjorn3 · 2023-02-08T11:25:23Z

cranelift/frontend/src/ssa.rs

+                table, destination, ..
            } => {
-                // In the case of a jump table, the situation is tricky because br_table doesn't
-                // support arguments. We have to split the critical edge.


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?

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 :)

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

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.

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

github-actions · 2023-02-09T01:51:17Z

Subscribe to Label Action

cc @cfallin, @fitzgen

Details

This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "cranelift:meta", "cranelift:wasm", "isle"

Thus the following users have been cc'd because of the following labels:

cfallin: isle
fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

cranelift/codegen/src/inst_predicates.rs

cranelift/codegen/src/isa/x64/mod.rs

…age (#5770) The new api on JumpTableData makese it easy to keep the default label first, and that shrinks the diff in #5731 a bit.

elliottt · 2023-02-14T17:02:23Z

cranelift/codegen/meta/src/cdsl/instructions.rs

-        if operand.is_immediate_or_entityref() {
+        if operand.kind.is_block() {
+            num_blocks += 1;
+        } else if operand.is_immediate_or_entityref() {


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.

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

elliottt · 2023-02-14T17:42:40Z

cranelift/interpreter/src/step.rs

-            let block = inst.branch_destination()[0];
-            continue_at(block)?
+            if let InstructionData::Jump { destination, .. } = inst {
+                continue_at(destination)?
+            } else {
+                unreachable!()
+            }


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

cranelift/codegen/src/ir/jumptable.rs

jameysharp

Looking good! I've been accumulating comments on this PR all afternoon in between other things and no longer have any idea what I've said, but hopefully it'll make sense.

jameysharp · 2023-02-14T23:14:51Z

cranelift/codegen/src/inst_predicates.rs

+                // so it is not part of the table. We visit the default block
+                // first explicitly, to mirror the traversal order of
+                // `JumpTableData::all_branches`, and transitively the order of


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

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 :)

jameysharp · 2023-02-14T23:29:20Z

cranelift/codegen/meta/src/cdsl/instructions.rs

-        if operand.is_immediate_or_entityref() {
+        if operand.kind.is_block() {
+            num_blocks += 1;
+        } else if operand.is_immediate_or_entityref() {


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

cranelift/codegen/src/ir/function.rs

cranelift/codegen/src/remove_constant_phis.rs

jameysharp · 2023-02-15T02:41:45Z

cranelift/fuzzgen/src/function_generator.rs

+                let mut jt = Vec::with_capacity(targets.len());
+                for block in targets {
+                    let args = self.generate_values_for_block(builder, block)?;
+                    jt.push(builder.func.dfg.block_call(block, &args))
+                }
+
+                let args = self.generate_values_for_block(builder, default)?;
+                let jt_data = JumpTableData::new(builder.func.dfg.block_call(default, &args), &jt);
+                let jt = builder.create_jump_table(jt_data);


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!

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:

wasmtime/cranelift/fuzzgen/src/function_generator.rs

Lines 1806 to 1815 in e10094d

// BrTable and the Switch interface only allow targeting blocks without params

// we also need to ensure that the next block has no params, since that one is

// guaranteed to be picked in either case.

if has_paramless_targets && next_block_is_paramless {

valid_terminators.extend_from_slice(&[

BlockTerminatorKind::BrTable,

BlockTerminatorKind::Switch,

]);

}

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!

Edit: Oops! It looks like the switch interface still does not allow blocks with params! So we may have to keep forward_blocks_without_params 😕 .

That being said, we should still try to select BrTable as often as we can and not just on paramless blocks.

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

jameysharp

Looks good, thank you! I have one style suggestion and I guess CI is failing, but I think this is basically ready.

cranelift/fuzzgen/src/function_generator.rs

cranelift/filetests/filetests/isa/x64/branches.clif

This has been unused since bytecodealliance#5731

This has been unused since #5731

elliottt force-pushed the trevor/br-table-block-args branch from 0b76e30 to c5da90a Compare February 7, 2023 01:17

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:meta Everything related to the meta-language. cranelift:wasm labels Feb 7, 2023

bjorn3 reviewed Feb 7, 2023

View reviewed changes

cranelift/codegen/meta/src/shared/instructions.rs Outdated Show resolved Hide resolved

elliottt force-pushed the trevor/br-table-block-args branch from c5da90a to 97a6f3a Compare February 7, 2023 21:31

elliottt mentioned this pull request Feb 8, 2023

Move jump tables to the DataFlowGraph #5745

Merged

elliottt force-pushed the trevor/br-table-block-args branch from 97a6f3a to 32e54d6 Compare February 8, 2023 05:33

bjorn3 reviewed Feb 8, 2023

View reviewed changes

github-actions bot added the cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. label Feb 8, 2023

elliottt force-pushed the trevor/br-table-block-args branch 2 times, most recently from 8894db9 to 7d2d126 Compare February 9, 2023 00:31

github-actions bot added the isle Related to the ISLE domain-specific language label Feb 9, 2023

elliottt force-pushed the trevor/br-table-block-args branch from 4433b1f to 2ab75f3 Compare February 9, 2023 17:55

elliottt mentioned this pull request Feb 9, 2023

Move default blocks into jump tables #5756

Merged

elliottt force-pushed the trevor/br-table-block-args branch 4 times, most recently from 051be27 to 36b5699 Compare February 10, 2023 18:04

elliottt commented Feb 13, 2023

View reviewed changes

cranelift/codegen/src/inst_predicates.rs Show resolved Hide resolved

elliottt commented Feb 13, 2023

View reviewed changes

cranelift/codegen/src/isa/x64/mod.rs Outdated Show resolved Hide resolved

elliottt mentioned this pull request Feb 13, 2023

Move the default block to the front of the underlying jump table storage #5770

Merged

elliottt force-pushed the trevor/br-table-block-args branch from 54ba215 to 9127cb1 Compare February 13, 2023 20:11

elliottt added a commit that referenced this pull request Feb 13, 2023

Move the default block to the front of the underlying jump table stor…

19f337e

…age (#5770) The new api on JumpTableData makese it easy to keep the default label first, and that shrinks the diff in #5731 a bit.

elliottt force-pushed the trevor/br-table-block-args branch 2 times, most recently from 97f706f to 7f362c8 Compare February 14, 2023 01:03

elliottt force-pushed the trevor/br-table-block-args branch from 7f362c8 to 05d217d Compare February 14, 2023 01:07

elliottt commented Feb 14, 2023

View reviewed changes

elliottt force-pushed the trevor/br-table-block-args branch from 05d217d to e3d084c Compare February 14, 2023 17:43

elliottt marked this pull request as ready for review February 14, 2023 17:43

elliottt requested a review from jameysharp February 14, 2023 17:43

bjorn3 reviewed Feb 14, 2023

View reviewed changes

cranelift/codegen/src/ir/jumptable.rs Show resolved Hide resolved

jameysharp reviewed Feb 15, 2023

View reviewed changes

elliottt requested a review from jameysharp February 15, 2023 18:15

jameysharp approved these changes Feb 15, 2023

View reviewed changes

cranelift/fuzzgen/src/function_generator.rs Outdated Show resolved Hide resolved

elliottt force-pushed the trevor/br-table-block-args branch from b697873 to ec4f81d Compare February 16, 2023 00:42

elliottt commented Feb 16, 2023

View reviewed changes

cranelift/filetests/filetests/isa/x64/branches.clif Show resolved Hide resolved

elliottt added 7 commits February 15, 2023 19:58

Rework br_table to take BlockCall arguments

b506181

Inline append_jump_argument

43561af

Add some block args to the jumptable test

03cb859

Review feedback

9860587

Remove arrayvec

83b7e13

Remove unused import

8882d1f

Use collect instead

a6f7a6e

elliottt force-pushed the trevor/br-table-block-args branch from ec4f81d to a6f7a6e Compare February 16, 2023 04:00

elliottt merged commit 80c147d into bytecodealliance:main Feb 16, 2023

bjorn3 added a commit to bjorn3/wasmtime that referenced this pull request Mar 17, 2023

Remove split_blocks_created field

4e3fb6a

This has been unused since bytecodealliance#5731

bjorn3 mentioned this pull request Mar 17, 2023

Remove split_blocks_created field #6044

Merged

elliottt pushed a commit that referenced this pull request Mar 17, 2023

Remove split_blocks_created field (#6044)

76c6ee7

This has been unused since #5731

	// BrTable and the Switch interface only allow targeting blocks without params
	// we also need to ensure that the next block has no params, since that one is
	// guaranteed to be picked in either case.
	if has_paramless_targets && next_block_is_paramless {
	valid_terminators.extend_from_slice(&[
	BlockTerminatorKind::BrTable,
	BlockTerminatorKind::Switch,
	]);
	}

Conversation

elliottt commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorn3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 9, 2023

Subscribe to Label Action

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jameysharp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afonso360 Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jameysharp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elliottt commented Feb 7, 2023 •

edited

Loading

afonso360 Feb 15, 2023 •

edited

Loading