Skip to content

Commit 78a4f9e

Browse files
kripkenkripken
authored
wasm2js2: optimize call_indirect and select operands (#2056)
Don't use temp vars to reorder them unless we need to.
1 parent 21f014f commit 78a4f9e

16 files changed

Lines changed: 330 additions & 144 deletions

‎scripts/test/env.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
2+
export const FUNCTION_TABLE = [];
3+
14
var tempRet0 = 0;
25

36
export function setTempRet0(x) {

‎src/ir/effects.h‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -59,21 +59,21 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer, OverriddenVisitor<Effe
5959

6060
// Helper functions to check for various effect types
6161

62-
bool accessesLocal() { return localsRead.size() + localsWritten.size() > 0; }
63-
bool accessesGlobal() { return globalsRead.size() + globalsWritten.size() > 0; }
64-
bool accessesMemory() { return calls || readsMemory || writesMemory; }
62+
bool accessesLocal() const { return localsRead.size() + localsWritten.size() > 0; }
63+
bool accessesGlobal() const { return globalsRead.size() + globalsWritten.size() > 0; }
64+
bool accessesMemory() const { return calls || readsMemory || writesMemory; }
6565

66-
bool hasGlobalSideEffects() { return calls || globalsWritten.size() > 0 || writesMemory || isAtomic; }
67-
bool hasSideEffects() { return hasGlobalSideEffects() || localsWritten.size() > 0 || branches || implicitTrap; }
68-
bool hasAnything() { return branches || calls || accessesLocal() || readsMemory || writesMemory || accessesGlobal() || implicitTrap || isAtomic; }
66+
bool hasGlobalSideEffects() const { return calls || globalsWritten.size() > 0 || writesMemory || isAtomic; }
67+
bool hasSideEffects() const { return hasGlobalSideEffects() || localsWritten.size() > 0 || branches || implicitTrap; }
68+
bool hasAnything() const { return branches || calls || accessesLocal() || readsMemory || writesMemory || accessesGlobal() || implicitTrap || isAtomic; }
6969

7070
bool noticesGlobalSideEffects() { return calls || readsMemory || isAtomic || globalsRead.size(); }
7171

7272
// check if we break to anything external from ourselves
7373
bool hasExternalBreakTargets() { return !breakNames.empty(); }
7474

7575
// checks if these effects would invalidate another set (e.g., if we write, we invalidate someone that reads, they can't be moved past us)
76-
bool invalidates(EffectAnalyzer& other) {
76+
bool invalidates(const EffectAnalyzer& other) {
7777
if ((branches && other.hasSideEffects()) ||
7878
(other.branches && hasSideEffects()) ||
7979
((writesMemory || calls) && other.accessesMemory()) ||

‎src/wasm2js.h‎

Lines changed: 89 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#include "mixed_arena.h"
3636
#include "asm_v_wasm.h"
3737
#include "abi/js.h"
38+
#include "ir/effects.h"
3839
#include "ir/find_all.h"
3940
#include "ir/import-utils.h"
4041
#include "ir/load-utils.h"
@@ -884,33 +885,54 @@ Ref Wasm2JSBuilder::processFunctionBody(Module* m, Function* func, bool standalo
884885
}
885886

886887
Ref visitCallIndirect(CallIndirect* curr) {
887-
// TODO: the codegen here is a pessimization of what the ideal codegen
888-
// looks like. Eventually if necessary this should be tightened up in the
889-
// case that the argument expression doesn't have any side effects.
890-
Ref ret;
891-
ScopedTemp idx(i32, parent, func);
892-
std::vector<ScopedTemp*> temps; // TODO: utility class, with destructor?
893-
for (auto& operand : curr->operands) {
894-
temps.push_back(new ScopedTemp(operand->type, parent, func));
895-
IString temp = temps.back()->temp;
896-
sequenceAppend(ret, visitAndAssign(operand, temp));
897-
}
898-
sequenceAppend(ret, visitAndAssign(curr->target, idx));
899-
Ref theCall = ValueBuilder::makeCall(ValueBuilder::makeSub(
900-
ValueBuilder::makeName(FUNCTION_TABLE),
901-
idx.getAstName()
902-
));
903-
for (size_t i = 0; i < temps.size(); i++) {
904-
IString temp = temps[i]->temp;
905-
auto &operand = curr->operands[i];
906-
theCall[2]->push_back(makeAsmCoercion(ValueBuilder::makeName(temp), wasmToAsmType(operand->type)));
888+
// If the target has effects that interact with the operands, we must reorder it to the start.
889+
bool mustReorder = false;
890+
EffectAnalyzer targetEffects(parent->options, curr->target);
891+
if (targetEffects.hasAnything()) {
892+
for (auto* operand : curr->operands) {
893+
if (targetEffects.invalidates(EffectAnalyzer(parent->options, operand))) {
894+
mustReorder = true;
895+
break;
896+
}
897+
}
907898
}
908-
theCall = makeAsmCoercion(theCall, wasmToAsmType(curr->type));
909-
sequenceAppend(ret, theCall);
910-
for (auto temp : temps) {
911-
delete temp;
899+
if (mustReorder) {
900+
Ref ret;
901+
ScopedTemp idx(i32, parent, func);
902+
std::vector<ScopedTemp*> temps; // TODO: utility class, with destructor?
903+
for (auto* operand : curr->operands) {
904+
temps.push_back(new ScopedTemp(operand->type, parent, func));
905+
IString temp = temps.back()->temp;
906+
sequenceAppend(ret, visitAndAssign(operand, temp));
907+
}
908+
sequenceAppend(ret, visitAndAssign(curr->target, idx));
909+
Ref theCall = ValueBuilder::makeCall(ValueBuilder::makeSub(
910+
ValueBuilder::makeName(FUNCTION_TABLE),
911+
idx.getAstName()
912+
));
913+
for (size_t i = 0; i < temps.size(); i++) {
914+
IString temp = temps[i]->temp;
915+
auto &operand = curr->operands[i];
916+
theCall[2]->push_back(makeAsmCoercion(ValueBuilder::makeName(temp), wasmToAsmType(operand->type)));
917+
}
918+
theCall = makeAsmCoercion(theCall, wasmToAsmType(curr->type));
919+
sequenceAppend(ret, theCall);
920+
for (auto temp : temps) {
921+
delete temp;
922+
}
923+
return ret;
924+
} else {
925+
// Target has no side effects, emit simple code
926+
Ref theCall = ValueBuilder::makeCall(ValueBuilder::makeSub(
927+
ValueBuilder::makeName(FUNCTION_TABLE),
928+
visit(curr->target, EXPRESSION_RESULT)
929+
));
930+
for (auto* operand : curr->operands) {
931+
theCall[2]->push_back(visit(operand, EXPRESSION_RESULT));
932+
}
933+
theCall = makeAsmCoercion(theCall, wasmToAsmType(curr->type));
934+
return theCall;
912935
}
913-
return ret;
914936
}
915937

916938
// TODO: remove
@@ -1526,28 +1548,49 @@ Ref Wasm2JSBuilder::processFunctionBody(Module* m, Function* func, bool standalo
15261548
}
15271549

15281550
Ref visitSelect(Select* curr) {
1529-
// normal select
1530-
ScopedTemp tempIfTrue(curr->type, parent, func),
1531-
tempIfFalse(curr->type, parent, func),
1532-
tempCondition(i32, parent, func);
1533-
Ref ifTrue = visit(curr->ifTrue, EXPRESSION_RESULT);
1534-
Ref ifFalse = visit(curr->ifFalse, EXPRESSION_RESULT);
1535-
Ref condition = visit(curr->condition, EXPRESSION_RESULT);
1536-
return
1537-
ValueBuilder::makeSeq(
1538-
ValueBuilder::makeBinary(tempIfTrue.getAstName(), SET, ifTrue),
1551+
// If the condition has effects that interact with the operands, we must reorder it to the start.
1552+
// We must also use locals if the values have side effects, as a JS conditional does not
1553+
// visit both sides.
1554+
bool useLocals = false;
1555+
EffectAnalyzer conditionEffects(parent->options, curr->condition);
1556+
EffectAnalyzer ifTrueEffects(parent->options, curr->ifTrue);
1557+
EffectAnalyzer ifFalseEffects(parent->options, curr->ifFalse);
1558+
if (conditionEffects.invalidates(ifTrueEffects) ||
1559+
conditionEffects.invalidates(ifFalseEffects) ||
1560+
ifTrueEffects.hasSideEffects() ||
1561+
ifFalseEffects.hasSideEffects()) {
1562+
useLocals = true;
1563+
}
1564+
if (useLocals) {
1565+
ScopedTemp tempIfTrue(curr->type, parent, func),
1566+
tempIfFalse(curr->type, parent, func),
1567+
tempCondition(i32, parent, func);
1568+
Ref ifTrue = visit(curr->ifTrue, EXPRESSION_RESULT);
1569+
Ref ifFalse = visit(curr->ifFalse, EXPRESSION_RESULT);
1570+
Ref condition = visit(curr->condition, EXPRESSION_RESULT);
1571+
return
15391572
ValueBuilder::makeSeq(
1540-
ValueBuilder::makeBinary(tempIfFalse.getAstName(), SET, ifFalse),
1573+
ValueBuilder::makeBinary(tempIfTrue.getAstName(), SET, ifTrue),
15411574
ValueBuilder::makeSeq(
1542-
ValueBuilder::makeBinary(tempCondition.getAstName(), SET, condition),
1543-
ValueBuilder::makeConditional(
1544-
tempCondition.getAstName(),
1545-
tempIfTrue.getAstName(),
1546-
tempIfFalse.getAstName()
1575+
ValueBuilder::makeBinary(tempIfFalse.getAstName(), SET, ifFalse),
1576+
ValueBuilder::makeSeq(
1577+
ValueBuilder::makeBinary(tempCondition.getAstName(), SET, condition),
1578+
ValueBuilder::makeConditional(
1579+
tempCondition.getAstName(),
1580+
tempIfTrue.getAstName(),
1581+
tempIfFalse.getAstName()
1582+
)
15471583
)
15481584
)
1549-
)
1585+
);
1586+
} else {
1587+
// Simple case without reordering.
1588+
return ValueBuilder::makeConditional(
1589+
visit(curr->condition, EXPRESSION_RESULT),
1590+
visit(curr->ifTrue, EXPRESSION_RESULT),
1591+
visit(curr->ifFalse, EXPRESSION_RESULT)
15501592
);
1593+
}
15511594
}
15521595

15531596
Ref visitReturn(Return* curr) {
@@ -1838,6 +1881,10 @@ void Wasm2JSGlue::emitPreES6() {
18381881
noteImport(import->module, import->base);
18391882
});
18401883

1884+
if (wasm.table.exists && wasm.table.imported()) {
1885+
out << "import { FUNCTION_TABLE } from 'env';\n";
1886+
}
1887+
18411888
out << '\n';
18421889
}
18431890

0 commit comments

Comments
 (0)