Skip to content

[GUFA] Simplify routines for dropping children (NFC) #4787

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

Merged
merged 29 commits into from
Jul 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
dc73f07
[Strings] string.measure (#4775)
kripken Jul 7, 2022
a82e2db
Fuzzer: Fix determinism fuzzing (#4767)
kripken Jul 7, 2022
9831b36
Group reference types in binary format. (#4774)
kripken Jul 7, 2022
19a4375
[Strings] string.encode (#4776)
kripken Jul 7, 2022
b88bedb
[Strings] string.concat (#4777)
kripken Jul 8, 2022
9c13144
Port a recently-updated test to lit (#4779)
kripken Jul 8, 2022
06c50a4
Add note on porting tests to README (#4782)
tlively Jul 8, 2022
838e8ee
[Parser] Parse standard subtype declarations (#4778)
tlively Jul 8, 2022
75d059a
[Strings] string.eq (#4781)
kripken Jul 8, 2022
fea11e9
[Strings] string.is_usv_sequence (#4783)
kripken Jul 8, 2022
395e9d4
Parse `rec` in update_lit_checks.py (#4784)
tlively Jul 8, 2022
83f48ed
[Parser] Parse rec groups (#4785)
tlively Jul 8, 2022
44fa122
[Wasm GC] RefIs / RefEq / RefTest return a boolean (#4786)
kripken Jul 8, 2022
95b8ea5
[GUFA] Simplify routines for dropping children (NFC)
aheejin Jul 9, 2022
3704951
Restore comments
aheejin Jul 9, 2022
0da2d8c
clang-format
aheejin Jul 9, 2022
e2ce69c
Fix binaryen.js to include allocate() explicitly (#4793)
kripken Jul 11, 2022
5aa2e18
[Parser] Start to parse instructions (#4789)
tlively Jul 11, 2022
483087c
4598
aheejin Jul 12, 2022
7988682
Merge branch '4598' into improve_remove
aheejin Jul 12, 2022
c00db9e
Revert "Fix binaryen.js to include allocate() explicitly (#4793)"
aheejin Jul 12, 2022
c6c0769
Revert "[Parser] Start to parse instructions (#4789)"
aheejin Jul 12, 2022
8f9e2a6
Revert "Revert "[Parser] Start to parse instructions (#4789)""
aheejin Jul 12, 2022
8955856
Revert "Revert "Fix binaryen.js to include allocate() explicitly (#47…
aheejin Jul 12, 2022
1eed7e0
Revert "Merge branch '4598' into improve_remove"
aheejin Jul 12, 2022
d556760
feedback
kripken Jul 11, 2022
14d83ef
Revert "feedback"
aheejin Jul 12, 2022
4a1ca02
Add `optimize = true`s
aheejin Jul 12, 2022
8925759
Test changes
aheejin Jul 12, 2022
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
28 changes: 27 additions & 1 deletion src/ir/drop.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#ifndef wasm_ir_drop_h
#define wasm_ir_drop_h

#include "ir/branch-utils.h"
#include "ir/effects.h"
#include "ir/iteration.h"
#include "wasm-builder.h"
Expand Down Expand Up @@ -94,12 +95,37 @@ inline Expression* getDroppedChildrenAndAppend(Expression* curr,
// )
// )
// (appended last item)
//
// Also this function preserves other unremovable expressions like trys and
// pops.
inline Expression*
getDroppedUnconditionalChildrenAndAppend(Expression* curr,
Module& wasm,
const PassOptions& options,
Expression* last) {
if (curr->is<If>() || curr->is<Try>()) {
// We check for shallow effects here, since we may be able to remove |curr|
// itself but keep its children around - we don't want effects in the children
// to stop us from improving the code. Note that there are cases where the
// combined curr+children has fewer effects than curr itself, such as if curr
// is a block and the child branches to it, but in such cases we cannot remove
// curr anyhow (those cases are ruled out below), so looking at non-shallow
// effects would never help us (and would be slower to run).
ShallowEffectAnalyzer effects(options, wasm, curr);
// Ignore a trap, as the unreachable replacement would trap too.
if (last->type == Type::unreachable) {
effects.trap = false;
}

// We cannot remove
// 1. Expressions with unremovable side effects
// 2. if: 'if's contains conditional expressions
// 3. try: Removing a try could leave a pop without a proper parent
// 4. pop: Pops are struturally necessary in catch bodies
// 5. Branch targets: We will need the target for the branches to it to
// validate.
if (effects.hasUnremovableSideEffects() || curr->is<If>() ||
curr->is<Try>() || curr->is<Pop>() ||
BranchUtils::getDefinedName(curr).is()) {
Builder builder(wasm);
return builder.makeSequence(builder.makeDrop(curr), last);
}
Expand Down
81 changes: 8 additions & 73 deletions src/passes/GUFA.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,52 +69,6 @@ struct GUFAOptimizer

bool optimized = false;

// Check if removing something (but not its children - just the node itself)
// would be ok structurally - whether the IR would still validate.
bool canRemoveStructurally(Expression* curr) {
// We can remove almost anything, but not a branch target, as we still need
// the target for the branches to it to validate.
if (BranchUtils::getDefinedName(curr).is()) {
return false;
}

// Pops are structurally necessary in catch bodies, and removing a try could
// leave a pop without a proper parent.
return !curr->is<Pop>() && !curr->is<Try>();
}

// Whether we can remove something (but not its children) without changing
// observable behavior or breaking validation.
bool canRemove(Expression* curr) {
if (!canRemoveStructurally(curr)) {
return false;
}

// We check for shallow effects here, since we may be able to remove |curr|
// itself but keep its children around - we don't want effects in the
// children to stop us from improving the code. Note that there are cases
// where the combined curr+children has fewer effects than curr itself,
// such as if curr is a block and the child branches to it, but in such
// cases we cannot remove curr anyhow (those cases are ruled out by
// canRemoveStructurally), so looking at non-shallow effects would never
// help us (and would be slower to run).
return !ShallowEffectAnalyzer(getPassOptions(), *getModule(), curr)
.hasUnremovableSideEffects();
}

// Whether we can replace something (but not its children, we can keep them
// with drops) with an unreachable without changing observable behavior or
// breaking validation.
bool canReplaceWithUnreachable(Expression* curr) {
if (!canRemoveStructurally(curr)) {
return false;
}
ShallowEffectAnalyzer effects(getPassOptions(), *getModule(), curr);
// Ignore a trap, as the unreachable replacement would trap too.
effects.trap = false;
return !effects.hasUnremovableSideEffects();
}

void visitExpression(Expression* curr) {
// Skip things we can't improve in any way.
auto type = curr->type;
Expand Down Expand Up @@ -143,23 +97,12 @@ struct GUFAOptimizer
auto& wasm = *getModule();
Builder builder(wasm);

auto replaceWithUnreachable = [&]() {
if (canReplaceWithUnreachable(curr)) {
replaceCurrent(getDroppedUnconditionalChildrenAndAppend(
curr, wasm, options, builder.makeUnreachable()));
} else {
// We can't remove this, but we can at least put an unreachable
// right after it.
replaceCurrent(builder.makeSequence(builder.makeDrop(curr),
builder.makeUnreachable()));
}
optimized = true;
};

if (contents.getType() == Type::unreachable) {
// This cannot contain any possible value at all. It must be unreachable
// code.
replaceWithUnreachable();
replaceCurrent(getDroppedUnconditionalChildrenAndAppend(
curr, wasm, options, builder.makeUnreachable()));
optimized = true;
return;
}

Expand Down Expand Up @@ -196,18 +139,8 @@ struct GUFAOptimizer
// ref.as etc. Once it does those we could assert on the type being
// valid here.
if (Type::isSubType(c->type, curr->type)) {
if (canRemove(curr)) {
replaceCurrent(
getDroppedUnconditionalChildrenAndAppend(curr, wasm, options, c));
} else {
// We can't remove this, but we can at least drop it and put the
// optimized value right after it.
// TODO: Should this only be done when not optimizing for size? In
// general this may increase code size unless later passes can
// remove the dropped |curr|, or unless the propagated constant is
// helpful in other opts.
replaceCurrent(builder.makeSequence(builder.makeDrop(curr), c));
}
replaceCurrent(
getDroppedUnconditionalChildrenAndAppend(curr, wasm, options, c));
optimized = true;
} else {
// The type is not compatible: we cannot place |c| in this location, even
Expand All @@ -216,7 +149,9 @@ struct GUFAOptimizer
// The type is not compatible and this is a simple constant expression
// like a ref.func. That means this code must be unreachable. (See below
// for the case of a non-constant.)
replaceWithUnreachable();
replaceCurrent(getDroppedUnconditionalChildrenAndAppend(
curr, wasm, options, builder.makeUnreachable()));
optimized = true;
} else {
// This is not a constant expression, but we are certain it is the right
// value. Atm the only such case we handle is a global.get of an
Expand Down