Skip to content

Commit 7d5d24f

Browse files
kripkenkripken
authored
Extend drop.h and use it in Directize (#5713)
This adds an option to ignore effects in the parent in getDroppedChildrenAndAppend. With that, this becomes usable in more places, like Directize, basically in situations where we know we can ignore effects in the parent (since we've inferred they are not needed). This lets us get rid of some boilerplate code in Directize. Diff without whitespace is a lot smaller. A large other part of the diff is a rename of curr => parent which I think it makes it more readable as then parent/children is a clear contrast, and then the new parameter "ignore/ notice parent effects" is obviously connected to "parent". The top comment in drop.cpp is removed as it just duplicated the top comment in the header drop.h. This is basically NFC but using drop.h does bring the advantage of emitting less code, see the test changes, so it is noticeable in the IR. This is a refactoring PR in preparation for a larger improvement to Directize that will also benefit from this new drop capability.
1 parent b1dba91 commit 7d5d24f

File tree

4 files changed

+60
-128
lines changed

4 files changed

+60
-128
lines changed

‎src/ir/drop.cpp‎

Lines changed: 24 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,7 @@
1414
* limitations under the License.
1515
*/
1616

17-
#ifndef wasm_ir_drop_h
18-
#define wasm_ir_drop_h
19-
17+
#include "ir/drop.h"
2018
#include "ir/branch-utils.h"
2119
#include "ir/effects.h"
2220
#include "ir/iteration.h"
@@ -25,27 +23,26 @@
2523

2624
namespace wasm {
2725

28-
// Given an expression, returns a new expression that drops the given
29-
// expression's children that cannot be removed outright due to their side
30-
// effects. Note that this only operates on children that execute
31-
// unconditionally. That is the case in almost all expressions, except for those
32-
// with conditional execution, like if, which unconditionally executes the
33-
// condition but then conditionally executes one of the two arms.
34-
Expression* getDroppedChildrenAndAppend(Expression* curr,
26+
Expression* getDroppedChildrenAndAppend(Expression* parent,
3527
Module& wasm,
3628
const PassOptions& options,
37-
Expression* last) {
38-
// We check for shallow effects here, since we may be able to remove |curr|
29+
Expression* last,
30+
DropMode mode) {
31+
// We check for shallow effects here, since we may be able to remove |parent|
3932
// itself but keep its children around - we don't want effects in the children
4033
// to stop us from improving the code. Note that there are cases where the
41-
// combined curr+children has fewer effects than curr itself, such as if curr
42-
// is a block and the child branches to it, but in such cases we cannot remove
43-
// curr anyhow (those cases are ruled out below), so looking at non-shallow
44-
// effects would never help us (and would be slower to run).
45-
ShallowEffectAnalyzer effects(options, wasm, curr);
46-
// Ignore a trap, as the unreachable replacement would trap too.
47-
if (last->is<Unreachable>()) {
48-
effects.trap = false;
34+
// combined parent+children has fewer effects than parent itself, such as if
35+
// parent is a block and the child branches to it, but in such cases we cannot
36+
// remove parent anyhow (those cases are ruled out below), so looking at
37+
// non-shallow effects would never help us (and would be slower to run).
38+
bool keepParent = false;
39+
if (mode == DropMode::NoticeParentEffects) {
40+
ShallowEffectAnalyzer effects(options, wasm, parent);
41+
// Ignore a trap, as the unreachable replacement would trap too.
42+
if (last->is<Unreachable>()) {
43+
effects.trap = false;
44+
}
45+
keepParent = effects.hasUnremovableSideEffects();
4946
}
5047

5148
// We cannot remove
@@ -56,19 +53,18 @@ Expression* getDroppedChildrenAndAppend(Expression* curr,
5653
// 5. Branch targets: We will need the target for the branches to it to
5754
// validate.
5855
Builder builder(wasm);
59-
if (effects.hasUnremovableSideEffects() || curr->is<If>() ||
60-
curr->is<Try>() || curr->is<Pop>() ||
61-
BranchUtils::getDefinedName(curr).is()) {
62-
// If curr is concrete we must drop it. Or, if it is unreachable or none,
56+
if (keepParent || parent->is<If>() || parent->is<Try>() ||
57+
parent->is<Pop>() || BranchUtils::getDefinedName(parent).is()) {
58+
// If parent is concrete we must drop it. Or, if it is unreachable or none,
6359
// then we can leave it as it is.
64-
if (curr->type.isConcrete()) {
65-
curr = builder.makeDrop(curr);
60+
if (parent->type.isConcrete()) {
61+
parent = builder.makeDrop(parent);
6662
}
67-
return builder.makeSequence(curr, last);
63+
return builder.makeSequence(parent, last);
6864
}
6965

7066
std::vector<Expression*> contents;
71-
for (auto* child : ChildIterator(curr)) {
67+
for (auto* child : ChildIterator(parent)) {
7268
if (!EffectAnalyzer(options, wasm, child).hasUnremovableSideEffects()) {
7369
continue;
7470
}
@@ -87,5 +83,3 @@ Expression* getDroppedChildrenAndAppend(Expression* curr,
8783
}
8884

8985
} // namespace wasm
90-
91-
#endif // wasm_ir_drop_h

‎src/ir/drop.h‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,19 @@ struct PassOptions;
7070
// )
7171
// (appended last item)
7272
//
73-
// Also this function preserves other unremovable expressions like trys, pops,
74-
// and named blocks.
75-
Expression* getDroppedChildrenAndAppend(Expression* curr,
76-
Module& wasm,
77-
const PassOptions& options,
78-
Expression* last);
73+
// Also this function preserves other unremovable expressions like trys, pops,
74+
// and named blocks.
75+
//
76+
// We can run in two modes: where we notice parent effects, and in that case we
77+
// won't remove effects there (we'll keep a drop of the parent too), or we can
78+
// ignore parent effects.
79+
enum class DropMode { NoticeParentEffects, IgnoreParentEffects };
80+
Expression*
81+
getDroppedChildrenAndAppend(Expression* parent,
82+
Module& wasm,
83+
const PassOptions& options,
84+
Expression* last,
85+
DropMode mode = DropMode::NoticeParentEffects);
7986

8087
} // namespace wasm
8188

‎src/passes/Directize.cpp‎

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
#include <unordered_map>
3434

3535
#include "call-utils.h"
36+
#include "ir/drop.h"
3637
#include "ir/table-utils.h"
3738
#include "ir/utils.h"
3839
#include "pass.h"
@@ -170,12 +171,18 @@ struct FunctionDirectizer : public WalkerPass<PostWalker<FunctionDirectizer>> {
170171
// We don't know anything here.
171172
return;
172173
}
173-
// If the index is invalid, or the type is wrong, we can
174-
// emit an unreachable here, since in Binaryen it is ok to
175-
// reorder/replace traps when optimizing (but never to
174+
// If the index is invalid, or the type is wrong, we can skip the call and
175+
// emit an unreachable here (with dropped children as needed), since in
176+
// Binaryen it is ok to reorder/replace traps when optimizing (but never to
176177
// remove them, at least not by default).
177178
if (std::get_if<CallUtils::Trap>(&info)) {
178-
replaceCurrent(replaceWithUnreachable(operands));
179+
replaceCurrent(
180+
getDroppedChildrenAndAppend(original,
181+
*getModule(),
182+
getPassOptions(),
183+
Builder(*getModule()).makeUnreachable(),
184+
DropMode::IgnoreParentEffects));
185+
changedTypes = true;
179186
return;
180187
}
181188

@@ -185,19 +192,6 @@ struct FunctionDirectizer : public WalkerPass<PostWalker<FunctionDirectizer>> {
185192
Builder(*getModule())
186193
.makeCall(name, operands, original->type, original->isReturn));
187194
}
188-
189-
Expression* replaceWithUnreachable(const std::vector<Expression*>& operands) {
190-
// Emitting an unreachable means we must update parent types.
191-
changedTypes = true;
192-
193-
Builder builder(*getModule());
194-
std::vector<Expression*> newOperands;
195-
for (auto* operand : operands) {
196-
newOperands.push_back(builder.makeDrop(operand));
197-
}
198-
return builder.makeSequence(builder.makeBlock(newOperands),
199-
builder.makeUnreachable());
200-
}
201195
};
202196

203197
struct Directize : public Pass {

‎test/lit/passes/directize_all-features.wast‎

Lines changed: 12 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -541,22 +541,16 @@
541541
(unreachable)
542542
)
543543
;; CHECK: (func $bar (type $ii) (param $x i32) (param $y i32)
544-
;; CHECK-NEXT: (block
545-
;; CHECK-NEXT: (drop
546-
;; CHECK-NEXT: (local.get $x)
547-
;; CHECK-NEXT: )
548-
;; CHECK-NEXT: (drop
544+
;; CHECK-NEXT: (drop
545+
;; CHECK-NEXT: (local.tee $y
549546
;; CHECK-NEXT: (local.get $y)
550547
;; CHECK-NEXT: )
551548
;; CHECK-NEXT: )
552549
;; CHECK-NEXT: (unreachable)
553550
;; CHECK-NEXT: )
554551
;; IMMUT: (func $bar (type $ii) (param $x i32) (param $y i32)
555-
;; IMMUT-NEXT: (block
556-
;; IMMUT-NEXT: (drop
557-
;; IMMUT-NEXT: (local.get $x)
558-
;; IMMUT-NEXT: )
559-
;; IMMUT-NEXT: (drop
552+
;; IMMUT-NEXT: (drop
553+
;; IMMUT-NEXT: (local.tee $y
560554
;; IMMUT-NEXT: (local.get $y)
561555
;; IMMUT-NEXT: )
562556
;; IMMUT-NEXT: )
@@ -565,7 +559,10 @@
565559
(func $bar (param $x i32) (param $y i32)
566560
(call_indirect (type $ii)
567561
(local.get $x)
568-
(local.get $y)
562+
;; Use a local.tee to show that an operand with side effects is kept/dropped.
563+
(local.tee $y
564+
(local.get $y)
565+
)
569566
(i32.const 5)
570567
)
571568
)
@@ -594,25 +591,9 @@
594591
(unreachable)
595592
)
596593
;; CHECK: (func $bar (type $ii) (param $x i32) (param $y i32)
597-
;; CHECK-NEXT: (block
598-
;; CHECK-NEXT: (drop
599-
;; CHECK-NEXT: (local.get $x)
600-
;; CHECK-NEXT: )
601-
;; CHECK-NEXT: (drop
602-
;; CHECK-NEXT: (local.get $y)
603-
;; CHECK-NEXT: )
604-
;; CHECK-NEXT: )
605594
;; CHECK-NEXT: (unreachable)
606595
;; CHECK-NEXT: )
607596
;; IMMUT: (func $bar (type $ii) (param $x i32) (param $y i32)
608-
;; IMMUT-NEXT: (block
609-
;; IMMUT-NEXT: (drop
610-
;; IMMUT-NEXT: (local.get $x)
611-
;; IMMUT-NEXT: )
612-
;; IMMUT-NEXT: (drop
613-
;; IMMUT-NEXT: (local.get $y)
614-
;; IMMUT-NEXT: )
615-
;; IMMUT-NEXT: )
616597
;; IMMUT-NEXT: (unreachable)
617598
;; IMMUT-NEXT: )
618599
(func $bar (param $x i32) (param $y i32)
@@ -651,25 +632,9 @@
651632
(unreachable)
652633
)
653634
;; CHECK: (func $bar (type $ii) (param $x i32) (param $y i32)
654-
;; CHECK-NEXT: (block
655-
;; CHECK-NEXT: (drop
656-
;; CHECK-NEXT: (local.get $x)
657-
;; CHECK-NEXT: )
658-
;; CHECK-NEXT: (drop
659-
;; CHECK-NEXT: (local.get $y)
660-
;; CHECK-NEXT: )
661-
;; CHECK-NEXT: )
662635
;; CHECK-NEXT: (unreachable)
663636
;; CHECK-NEXT: )
664637
;; IMMUT: (func $bar (type $ii) (param $x i32) (param $y i32)
665-
;; IMMUT-NEXT: (block
666-
;; IMMUT-NEXT: (drop
667-
;; IMMUT-NEXT: (local.get $x)
668-
;; IMMUT-NEXT: )
669-
;; IMMUT-NEXT: (drop
670-
;; IMMUT-NEXT: (local.get $y)
671-
;; IMMUT-NEXT: )
672-
;; IMMUT-NEXT: )
673638
;; IMMUT-NEXT: (unreachable)
674639
;; IMMUT-NEXT: )
675640
(func $bar (param $x i32) (param $y i32)
@@ -710,19 +675,11 @@
710675
(table $0 8 8 funcref)
711676
;; CHECK: (func $0 (type $none_=>_none)
712677
;; CHECK-NEXT: (nop)
713-
;; CHECK-NEXT: (block
714-
;; CHECK-NEXT: (block
715-
;; CHECK-NEXT: )
716-
;; CHECK-NEXT: (unreachable)
717-
;; CHECK-NEXT: )
678+
;; CHECK-NEXT: (unreachable)
718679
;; CHECK-NEXT: )
719680
;; IMMUT: (func $0 (type $none_=>_none)
720681
;; IMMUT-NEXT: (nop)
721-
;; IMMUT-NEXT: (block
722-
;; IMMUT-NEXT: (block
723-
;; IMMUT-NEXT: )
724-
;; IMMUT-NEXT: (unreachable)
725-
;; IMMUT-NEXT: )
682+
;; IMMUT-NEXT: (unreachable)
726683
;; IMMUT-NEXT: )
727684
(func $0
728685
(block ;; the type of this block will change
@@ -1444,32 +1401,12 @@
14441401
;; CHECK-NEXT: )
14451402
;; CHECK-NEXT: )
14461403
;; IMMUT: (func $bar (type $ii) (param $x i32) (param $y i32)
1447-
;; IMMUT-NEXT: (block
1448-
;; IMMUT-NEXT: (block
1449-
;; IMMUT-NEXT: (drop
1450-
;; IMMUT-NEXT: (local.get $x)
1451-
;; IMMUT-NEXT: )
1452-
;; IMMUT-NEXT: (drop
1453-
;; IMMUT-NEXT: (local.get $y)
1454-
;; IMMUT-NEXT: )
1455-
;; IMMUT-NEXT: )
1456-
;; IMMUT-NEXT: (unreachable)
1457-
;; IMMUT-NEXT: )
1404+
;; IMMUT-NEXT: (unreachable)
14581405
;; IMMUT-NEXT: (call $foo1
14591406
;; IMMUT-NEXT: (local.get $x)
14601407
;; IMMUT-NEXT: (local.get $y)
14611408
;; IMMUT-NEXT: )
1462-
;; IMMUT-NEXT: (block
1463-
;; IMMUT-NEXT: (block
1464-
;; IMMUT-NEXT: (drop
1465-
;; IMMUT-NEXT: (local.get $x)
1466-
;; IMMUT-NEXT: )
1467-
;; IMMUT-NEXT: (drop
1468-
;; IMMUT-NEXT: (local.get $y)
1469-
;; IMMUT-NEXT: )
1470-
;; IMMUT-NEXT: )
1471-
;; IMMUT-NEXT: (unreachable)
1472-
;; IMMUT-NEXT: )
1409+
;; IMMUT-NEXT: (unreachable)
14731410
;; IMMUT-NEXT: (call $foo2
14741411
;; IMMUT-NEXT: (local.get $x)
14751412
;; IMMUT-NEXT: (local.get $y)

0 commit comments

Comments
 (0)