Skip to content

Commit f27ac28

Browse files
jaro-sevcikCommit Bot
jaro-sevcik
authored and
Commit Bot
committed
[turbofan] Pin pure unreachable values to effect chain (in rep selection)
Currently, if we lower to a pure computation that is unreachable because of some runtime check, we just rename it with DeadValue. This is problematic if the pure computation gets later eliminated - that allows the DeadValue node float above the check that makes it dead. As we conservatively lower DeadValues to debug-break (i.e., crash), we might induce crash where we should not. With this CL, whenever we lower an impossible effectful node (i.e., with Type::None) to a pure node in simplified lowering, we insert an Unreachable node there (pinned to the effect chain) and mark the impossible node dead (and make it depend on the Unreachable node). Bug: chromium:910838 Change-Id: I218991c79b9e283a9dd5beb4d3f0c4664be76cb2 Reviewed-on: https://chromium-review.googlesource.com/c/1365274 Reviewed-by: Benedikt Meurer <[email protected]> Commit-Queue: Jaroslav Sevcik <[email protected]> Cr-Commit-Position: refs/heads/master@{#58066}
1 parent f7f18b0 commit f27ac28

3 files changed

Lines changed: 72 additions & 35 deletions

File tree

‎src/compiler/simplified-lowering.cc‎

Lines changed: 52 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -172,21 +172,6 @@ void ReplaceEffectControlUses(Node* node, Node* effect, Node* control) {
172172
}
173173
}
174174

175-
void ChangeToPureOp(Node* node, const Operator* new_op) {
176-
DCHECK(new_op->HasProperty(Operator::kPure));
177-
if (node->op()->EffectInputCount() > 0) {
178-
DCHECK_LT(0, node->op()->ControlInputCount());
179-
// Disconnect the node from effect and control chains.
180-
Node* control = NodeProperties::GetControlInput(node);
181-
Node* effect = NodeProperties::GetEffectInput(node);
182-
ReplaceEffectControlUses(node, effect, control);
183-
node->TrimInputCount(new_op->ValueInputCount());
184-
} else {
185-
DCHECK_EQ(0, node->op()->ControlInputCount());
186-
}
187-
NodeProperties::ChangeOp(node, new_op);
188-
}
189-
190175
bool CanOverflowSigned32(const Operator* op, Type left, Type right,
191176
Zone* type_zone) {
192177
// We assume the inputs are checked Signed32 (or known statically
@@ -758,6 +743,31 @@ class RepresentationSelector {
758743
!GetUpperBound(node->InputAt(1)).Maybe(type);
759744
}
760745

746+
void ChangeToPureOp(Node* node, const Operator* new_op) {
747+
DCHECK(new_op->HasProperty(Operator::kPure));
748+
if (node->op()->EffectInputCount() > 0) {
749+
DCHECK_LT(0, node->op()->ControlInputCount());
750+
Node* control = NodeProperties::GetControlInput(node);
751+
Node* effect = NodeProperties::GetEffectInput(node);
752+
if (TypeOf(node).IsNone()) {
753+
// If the node is unreachable, insert an Unreachable node and mark the
754+
// value dead.
755+
// TODO(jarin,tebbi) Find a way to unify/merge this insertion with
756+
// InsertUnreachableIfNecessary.
757+
Node* unreachable = effect = graph()->NewNode(
758+
jsgraph_->common()->Unreachable(), effect, control);
759+
new_op = jsgraph_->common()->DeadValue(GetInfo(node)->representation());
760+
node->ReplaceInput(0, unreachable);
761+
}
762+
// Rewire the effect and control chains.
763+
node->TrimInputCount(new_op->ValueInputCount());
764+
ReplaceEffectControlUses(node, effect, control);
765+
} else {
766+
DCHECK_EQ(0, node->op()->ControlInputCount());
767+
}
768+
NodeProperties::ChangeOp(node, new_op);
769+
}
770+
761771
// Converts input {index} of {node} according to given UseInfo {use},
762772
// assuming the type of the input is {input_type}. If {input_type} is null,
763773
// it takes the input from the input node {TypeOf(node->InputAt(index))}.
@@ -1062,6 +1072,15 @@ class RepresentationSelector {
10621072
}
10631073
}
10641074

1075+
void MaskShiftOperand(Node* node, Type rhs_type) {
1076+
if (!rhs_type.Is(type_cache_.kZeroToThirtyOne)) {
1077+
Node* const rhs = NodeProperties::GetValueInput(node, 1);
1078+
node->ReplaceInput(1,
1079+
graph()->NewNode(jsgraph_->machine()->Word32And(), rhs,
1080+
jsgraph_->Int32Constant(0x1F)));
1081+
}
1082+
}
1083+
10651084
static MachineSemantic DeoptValueSemanticOf(Type type) {
10661085
// We only need signedness to do deopt correctly.
10671086
if (type.Is(Type::Signed32())) {
@@ -2112,7 +2131,8 @@ class RepresentationSelector {
21122131
VisitBinop(node, UseInfo::TruncatingWord32(),
21132132
UseInfo::TruncatingWord32(), MachineRepresentation::kWord32);
21142133
if (lower()) {
2115-
lowering->DoShift(node, lowering->machine()->Word32Shl(), rhs_type);
2134+
MaskShiftOperand(node, rhs_type);
2135+
ChangeToPureOp(node, lowering->machine()->Word32Shl());
21162136
}
21172137
return;
21182138
}
@@ -2123,7 +2143,8 @@ class RepresentationSelector {
21232143
UseInfo::TruncatingWord32(),
21242144
MachineRepresentation::kWord32);
21252145
if (lower()) {
2126-
lowering->DoShift(node, lowering->machine()->Word32Shl(), rhs_type);
2146+
MaskShiftOperand(node, rhs_type);
2147+
ChangeToPureOp(node, lowering->machine()->Word32Shl());
21272148
}
21282149
return;
21292150
}
@@ -2132,7 +2153,8 @@ class RepresentationSelector {
21322153
VisitBinop(node, CheckedUseInfoAsWord32FromHint(hint),
21332154
MachineRepresentation::kWord32, Type::Signed32());
21342155
if (lower()) {
2135-
lowering->DoShift(node, lowering->machine()->Word32Shl(), rhs_type);
2156+
MaskShiftOperand(node, rhs_type);
2157+
ChangeToPureOp(node, lowering->machine()->Word32Shl());
21362158
}
21372159
return;
21382160
}
@@ -2141,7 +2163,8 @@ class RepresentationSelector {
21412163
VisitBinop(node, UseInfo::TruncatingWord32(),
21422164
UseInfo::TruncatingWord32(), MachineRepresentation::kWord32);
21432165
if (lower()) {
2144-
lowering->DoShift(node, lowering->machine()->Word32Sar(), rhs_type);
2166+
MaskShiftOperand(node, rhs_type);
2167+
ChangeToPureOp(node, lowering->machine()->Word32Sar());
21452168
}
21462169
return;
21472170
}
@@ -2152,7 +2175,8 @@ class RepresentationSelector {
21522175
UseInfo::TruncatingWord32(),
21532176
MachineRepresentation::kWord32);
21542177
if (lower()) {
2155-
lowering->DoShift(node, lowering->machine()->Word32Sar(), rhs_type);
2178+
MaskShiftOperand(node, rhs_type);
2179+
ChangeToPureOp(node, lowering->machine()->Word32Sar());
21562180
}
21572181
return;
21582182
}
@@ -2161,7 +2185,8 @@ class RepresentationSelector {
21612185
VisitBinop(node, CheckedUseInfoAsWord32FromHint(hint),
21622186
MachineRepresentation::kWord32, Type::Signed32());
21632187
if (lower()) {
2164-
lowering->DoShift(node, lowering->machine()->Word32Sar(), rhs_type);
2188+
MaskShiftOperand(node, rhs_type);
2189+
ChangeToPureOp(node, lowering->machine()->Word32Sar());
21652190
}
21662191
return;
21672192
}
@@ -2170,7 +2195,8 @@ class RepresentationSelector {
21702195
VisitBinop(node, UseInfo::TruncatingWord32(),
21712196
UseInfo::TruncatingWord32(), MachineRepresentation::kWord32);
21722197
if (lower()) {
2173-
lowering->DoShift(node, lowering->machine()->Word32Shr(), rhs_type);
2198+
MaskShiftOperand(node, rhs_type);
2199+
ChangeToPureOp(node, lowering->machine()->Word32Shr());
21742200
}
21752201
return;
21762202
}
@@ -2199,14 +2225,16 @@ class RepresentationSelector {
21992225
UseInfo::TruncatingWord32(),
22002226
MachineRepresentation::kWord32);
22012227
if (lower()) {
2202-
lowering->DoShift(node, lowering->machine()->Word32Shr(), rhs_type);
2228+
MaskShiftOperand(node, rhs_type);
2229+
ChangeToPureOp(node, lowering->machine()->Word32Shr());
22032230
}
22042231
return;
22052232
}
22062233
VisitBinop(node, CheckedUseInfoAsWord32FromHint(hint),
22072234
MachineRepresentation::kWord32, Type::Unsigned32());
22082235
if (lower()) {
2209-
lowering->DoShift(node, lowering->machine()->Word32Shr(), rhs_type);
2236+
MaskShiftOperand(node, rhs_type);
2237+
ChangeToPureOp(node, lowering->machine()->Word32Shr());
22102238
}
22112239
return;
22122240
}
@@ -4000,16 +4028,6 @@ void SimplifiedLowering::DoMin(Node* node, Operator const* op,
40004028
NodeProperties::ChangeOp(node, common()->Select(rep));
40014029
}
40024030

4003-
void SimplifiedLowering::DoShift(Node* node, Operator const* op,
4004-
Type rhs_type) {
4005-
if (!rhs_type.Is(type_cache_.kZeroToThirtyOne)) {
4006-
Node* const rhs = NodeProperties::GetValueInput(node, 1);
4007-
node->ReplaceInput(1, graph()->NewNode(machine()->Word32And(), rhs,
4008-
jsgraph()->Int32Constant(0x1F)));
4009-
}
4010-
ChangeToPureOp(node, op);
4011-
}
4012-
40134031
void SimplifiedLowering::DoIntegral32ToBit(Node* node) {
40144032
Node* const input = node->InputAt(0);
40154033
Node* const zero = jsgraph()->Int32Constant(0);

‎src/compiler/simplified-lowering.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ class V8_EXPORT_PRIVATE SimplifiedLowering final {
3737
Node* node, RepresentationSelector* selector);
3838
void DoJSToNumberOrNumericTruncatesToWord32(Node* node,
3939
RepresentationSelector* selector);
40-
void DoShift(Node* node, Operator const* op, Type rhs_type);
4140
void DoIntegral32ToBit(Node* node);
4241
void DoOrderedNumberToBit(Node* node);
4342
void DoNumberToBit(Node* node);

‎test/mjsunit/compiler/regress-910838.js‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
// Copyright 2018 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
// Flags: --allow-natives-syntax
6+
7+
function f(b, s, x) {
8+
if (!b) {
9+
return (x ? b : s * undefined) ? 1 : 42;
10+
}
11+
}
12+
13+
function g(b, x) {
14+
return f(b, 'abc', x);
15+
}
16+
17+
f(false, 0, 0);
18+
g(true, 0);
19+
%OptimizeFunctionOnNextCall(g);
20+
assertEquals(42, g(false, 0));

0 commit comments

Comments
 (0)