Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Outlining] Fixes break reconstruction #6352

Merged
merged 7 commits into from
Feb 28, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/passes/Outlining.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,15 @@ struct ReconstructStringifyWalker
: state == NotInSeq ? &existingBuilder
: nullptr;
if (builder) {
ASSERT_OK(builder->visit(curr));
if (auto* expr = curr->dynCast<Break>()) {
Type type = expr->value ? expr->value->type : Type::none;
ASSERT_OK(builder->visitBreakWithType(expr, type));
} else if (auto* expr = curr->dynCast<Switch>()) {
Type type = expr->value ? expr->value->type : Type::none;
ASSERT_OK(builder->visitSwitchWithType(expr, type));
} else {
ASSERT_OK(builder->visit(curr));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add an assertion somewhere to future-proof ourselves against the possibility that a new kind of Expression starts to need a visit*WithType variant. This could either be an assert(!Properties::isBranch(curr)) here guarding the default builder->visit(curr) case, or it could be an assertion that curr is a Break or Switch inside IRBuilder::getBranchValue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
}
DBG(printVisitExpression(curr));

Expand Down
16 changes: 16 additions & 0 deletions src/wasm-ir-builder.h
Original file line number Diff line number Diff line change
Expand Up @@ -220,10 +220,26 @@ class IRBuilder : public UnifiedExpressionVisitor<IRBuilder, Result<>> {
[[nodiscard]] Result<> visitStructNew(StructNew*);
[[nodiscard]] Result<> visitArrayNew(ArrayNew*);
[[nodiscard]] Result<> visitArrayNewFixed(ArrayNewFixed*);
// Used to visit break exprs when traversing the module in the fully nested
// format. Break label destinations are assumed to have already been visited,
// with a corresponding push onto the scope stack. As a result, an exception
// will occur if a corresponding scope is not found for the break.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not an exception, right? Will we return an error in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, not an exception. Updated!

[[nodiscard]] Result<> visitBreak(Break*,
std::optional<Index> label = std::nullopt);
// Used to visit break nodes when traversing the module in the stacky format.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be more precise.

Suggested change
// Used to visit break nodes when traversing the module in the stacky format.
// Used to visit break nodes when traversing a single block without its context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// The type indicates how many values need to be popped from the stack and
// consumed by the break.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's important that the condition (if any), which is also popped from the stack, is not included in this.

Suggested change
// The type indicates how many values need to be popped from the stack and
// consumed by the break.
// The type indicates how many values the break carries to its destination.

Similar comments apply to visitSwitchWithType below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

[[nodiscard]] Result<> visitBreakWithType(Break*, Type);
[[nodiscard]] Result<>
// Used to visit switch exprs when traversing the module in the fully nested
// format. Switch label destinations are assumed to have already been visited,
// with a corresponding push onto the scope stack. As a result, an exception
// will occur if a corresponding scope is not found for the switch.
visitSwitch(Switch*, std::optional<Index> defaultLabel = std::nullopt);
// Used to visit switch nodes when traversing the module in the stacky format.
// The type indicates how many values need to be popped from the stack and
// consumed by the switch.
[[nodiscard]] Result<> visitSwitchWithType(Switch*, Type);
[[nodiscard]] Result<> visitCall(Call*);
[[nodiscard]] Result<> visitCallIndirect(CallIndirect*);
[[nodiscard]] Result<> visitCallRef(CallRef*);
Expand Down
38 changes: 38 additions & 0 deletions src/wasm/wasm-ir-builder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -431,6 +431,26 @@ Result<> IRBuilder::visitBreak(Break* curr, std::optional<Index> label) {
return Ok{};
}

Result<> IRBuilder::visitBreakWithType(Break* curr, Type type) {
if (curr->condition) {
auto cond = pop();
CHECK_ERR(cond);
curr->condition = *cond;
}
if (!curr->value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type should be the only source of truth here.

Suggested change
if (!curr->value) {
if (type == Type::None) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

curr->value = nullptr;
} else {
auto value = pop(type.size());
CHECK_ERR(value)
curr->value = *value;
}
// TODO: Call more efficient versions of finalize() that take the known type
// for other kinds of nodes as well, as done above.
ReFinalizeNode{}.visit(curr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we know what kind of expression this is, we can just call finalize directly. There's also no more efficient variant for Break.

Suggested change
// TODO: Call more efficient versions of finalize() that take the known type
// for other kinds of nodes as well, as done above.
ReFinalizeNode{}.visit(curr);
curr->finalize();

Same for Switch below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

push(curr);
return Ok{};
}

Result<> IRBuilder::visitSwitch(Switch* curr,
std::optional<Index> defaultLabel) {
auto cond = pop();
Expand All @@ -442,6 +462,24 @@ Result<> IRBuilder::visitSwitch(Switch* curr,
return Ok{};
}

Result<> IRBuilder::visitSwitchWithType(Switch* curr, Type type) {
auto cond = pop();
CHECK_ERR(cond);
curr->condition = *cond;
if (!curr->value) {
curr->value = nullptr;
} else {
auto value = pop(type.size());
CHECK_ERR(value)
curr->value = *value;
}
// TODO: Call more efficient versions of finalize() that take the known type
// for other kinds of nodes as well, as done above.
ReFinalizeNode{}.visit(curr);
push(curr);
return Ok{};
}

Result<> IRBuilder::visitCall(Call* curr) {
auto numArgs = wasm.getFunction(curr->target)->getNumParams();
curr->operands.resize(numArgs);
Expand Down
42 changes: 42 additions & 0 deletions test/lit/passes/outlining.wast
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,48 @@
)
)

(module
;; CHECK: (type $0 (func))

;; CHECK: (func $outline$ (type $0)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.const 2)
;; CHECK-NEXT: )
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: )

;; CHECK: (func $a (type $0)
;; CHECK-NEXT: (block $label1
;; CHECK-NEXT: (call $outline$)
;; CHECK-NEXT: (loop $loop-in
;; CHECK-NEXT: (br $label1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (call $outline$)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add a test for the br_table case as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

(block $label1
(drop
(i32.const 2)
)
(drop
(i32.const 1)
)
(loop
(br $label1)
)
(drop
(i32.const 2)
)
(drop
(i32.const 1)
)
)
)
)

;; Tests return instructions are correctly filtered from being outlined.
(module
;; CHECK: (type $0 (func (result i32)))
Expand Down
Loading