diff options
| author | Alon Zakai <azakai@google.com> | 2021-04-15 16:18:06 -0700 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2021-04-15 16:18:06 -0700 |
| commit | bd2e8661a31aa02f701e31110108a5f5c194afed (patch) | |
| tree | 15ad40486ec605598d2f3580a580d4f5fe4fc756 /src/passes/Precompute.cpp | |
| parent | ac2a49bc53d4b0817657aed9f89f33868e5ab3b9 (diff) | |
| download | binaryen-bd2e8661a31aa02f701e31110108a5f5c194afed.tar.gz binaryen-bd2e8661a31aa02f701e31110108a5f5c194afed.tar.bz2 binaryen-bd2e8661a31aa02f701e31110108a5f5c194afed.zip | |
[Wasm GC] Fix precompute on GC data (#3810)
This fixes precomputation on GC after #3803 was too optimistic.
The issue is subtle. Precompute will repeatedly evaluate expressions and
propagate their values, flowing them around, and it ignores side effects
when doing so. For example:
(block
..side effect..
(i32.const 1)
)
When we evaluate that we see there are side effects, but regardless of them
we know the value flowing out is 1. So we can propagate that value, if it is
assigned to a local and read elsewhere.
This is not valid for GC because struct.new and array.new have a "side
effect" that is noticeable in the result. Each time we call struct.new we get a
new struct with a new address, which ref.eq can distinguish. So when this
pass evaluates the same thing multiple times it will get a different result.
Also, we can't precompute a struct.get even if we know the struct, not unless
we know the reference has not escaped (where a call could modify it).
To avoid all that, do not precompute references, aside from the trivially safe ones
like nulls and function references (simple constants that are the same each time
we evaluate the expression emitting them).
precomputeExpression() had a minor bug which this fixes. It checked the type
of the expression to see if we can create a constant for it, but really it should
check the value - since (separate from this PR) we have no way to emit a
"constant" for a struct etc. Also that only matters if replaceExpression is true, that
is, if we are replacing with a constant; if we just want the value internally, we have
no limit on that.
Also add Literal support for comparing GC refs, which is used by ref.eq. Without
that tiny fix the tests here crash.
This adds a bunch of tests, many for corner cases that we don't handle (since
the PR makes us not propagate GC references). But they should be helpful
if/when we do, to avoid the mistakes in #3803
Diffstat (limited to 'src/passes/Precompute.cpp')
| -rw-r--r-- | src/passes/Precompute.cpp | 54 |
1 files changed, 41 insertions, 13 deletions
diff --git a/src/passes/Precompute.cpp b/src/passes/Precompute.cpp index 85e09f875..c07286ec9 100644 --- a/src/passes/Precompute.cpp +++ b/src/passes/Precompute.cpp @@ -87,6 +87,15 @@ public: return ConstantExpressionRunner< PrecomputingExpressionRunner>::visitLocalGet(curr); } + + // Heap data may be modified in ways we do not see. We would need escape + // analysis to avoid that risk. For now, disallow all heap operations. + // TODO: immutability might also be good enough + Flow visitStructNew(StructNew* curr) { return Flow(NONCONSTANT_FLOW); } + Flow visitStructGet(StructGet* curr) { return Flow(NONCONSTANT_FLOW); } + Flow visitArrayNew(ArrayNew* curr) { return Flow(NONCONSTANT_FLOW); } + Flow visitArrayGet(ArrayGet* curr) { return Flow(NONCONSTANT_FLOW); } + Flow visitArrayLen(ArrayLen* curr) { return Flow(NONCONSTANT_FLOW); } }; struct Precompute @@ -226,16 +235,21 @@ private: // Precompute an expression, returning a flow, which may be a constant // (that we can replace the expression with if replaceExpression is set). Flow precomputeExpression(Expression* curr, bool replaceExpression = true) { - if (!canEmitConstantFor(curr->type)) { - return Flow(NONCONSTANT_FLOW); - } + Flow flow; try { - return PrecomputingExpressionRunner( - getModule(), getValues, replaceExpression) - .visit(curr); + flow = + PrecomputingExpressionRunner(getModule(), getValues, replaceExpression) + .visit(curr); } catch (PrecomputingExpressionRunner::NonconstantException&) { return Flow(NONCONSTANT_FLOW); } + // If we are replacing the expression, then the resulting value must be of + // a type we can emit a constant for. + if (!flow.breaking() && replaceExpression && + !canEmitConstantFor(flow.values)) { + return Flow(NONCONSTANT_FLOW); + } + return flow; } // Precomputes the value of an expression, as opposed to the expression @@ -286,6 +300,17 @@ private: if (setValues[set].isConcrete()) { continue; // already known constant } + // Precompute the value. Note that this executes the code from scratch + // each time we reach this point, and so we need to be careful about + // repeating side effects if those side effects are expressed *in the + // value*. A case where that can happen is GC data (each struct.new + // creates a new, unique struct, even if the data is equal), and so + // PrecomputingExpressionRunner will return a nonconstant flow for all + // GC heap operations. + // (Other side effects are fine; if an expression does a call and we + // somehow know the entire expression precomputes to a 42, then we can + // propagate that 42 along to the users, regardless of whatever the call + // did globally.) auto values = setValues[set] = precomputeValue(Properties::getFallthrough( set->value, getPassOptions(), getModule()->features)); @@ -360,19 +385,22 @@ private: if (value.isNull()) { return true; } + return canEmitConstantFor(value.type); + } + + bool canEmitConstantFor(Type type) { // A function is fine to emit a constant for - we'll emit a RefFunc, which // is compact and immutable, so there can't be a problem. - if (value.type.isFunction()) { + if (type.isFunction()) { return true; } - // All other reference types cannot be precomputed. - if (value.type.isRef()) { + // All other reference types cannot be precomputed. Even an immutable GC + // reference is not currently something this pass can handle, as it will + // evaluate and reevaluate code multiple times in e.g. optimizeLocals, see + // the comment above. + if (type.isRef()) { return false; } - return canEmitConstantFor(value.type); - } - - bool canEmitConstantFor(Type type) { // For now, don't try to precompute an Rtt. TODO figure out when that would // be safe and useful. return !type.isRtt(); |