Skip to content

Commit

Permalink
[turbofan] Do not reduce Return nodes with multiple value inputs.
Browse files Browse the repository at this point in the history
The existing implementation assumes that return nodes have exactly one
real value input. This assumption does not hold for WebAssembly. To
avoid incorrect behavior, this CL turns of the reduction of returns
with a value input count != 1.

[email protected], [email protected]

Review-Url: https://codereview.chromium.org/2638053002
Cr-Commit-Position: refs/heads/master@{#42425}
  • Loading branch information
gahaas authored and Commit bot committed Jan 17, 2017
1 parent 5e31df2 commit cfa6ce3
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 1 deletion.
6 changes: 5 additions & 1 deletion src/compiler/common-operator-reducer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,6 @@ Reduction CommonOperatorReducer::ReducePhi(Node* node) {

Reduction CommonOperatorReducer::ReduceReturn(Node* node) {
DCHECK_EQ(IrOpcode::kReturn, node->opcode());
Node* const value = node->InputAt(1);
Node* effect = NodeProperties::GetEffectInput(node);
Node* const control = NodeProperties::GetControlInput(node);
bool changed = false;
Expand All @@ -298,6 +297,11 @@ Reduction CommonOperatorReducer::ReduceReturn(Node* node) {
NodeProperties::ReplaceEffectInput(node, effect);
changed = true;
}
// TODO(ahaas): Extend the reduction below to multiple return values.
if (ValueInputCountOfReturn(node->op()) != 1) {
return NoChange();
}
Node* const value = node->InputAt(1);
if (value->opcode() == IrOpcode::kPhi &&
NodeProperties::GetControlInput(value) == control &&
effect->opcode() == IrOpcode::kEffectPhi &&
Expand Down
7 changes: 7 additions & 0 deletions src/compiler/common-operator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,13 @@ DeoptimizeReason DeoptimizeReasonOf(Operator const* const op) {
return OpParameter<DeoptimizeReason>(op);
}

int ValueInputCountOfReturn(Operator const* const op) {
DCHECK(op->opcode() == IrOpcode::kReturn);
// Return nodes have a hidden input at index 0 which we ignore in the value
// input count.
return op->ValueInputCount() - 1;
}

size_t hash_value(DeoptimizeKind kind) { return static_cast<size_t>(kind); }

std::ostream& operator<<(std::ostream& os, DeoptimizeKind kind) {
Expand Down
3 changes: 3 additions & 0 deletions src/compiler/common-operator.h
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ V8_EXPORT_PRIVATE BranchHint BranchHintOf(const Operator* const);
// Deoptimize reason for Deoptimize, DeoptimizeIf and DeoptimizeUnless.
DeoptimizeReason DeoptimizeReasonOf(Operator const* const);

// Helper function for return nodes, because returns have a hidden value input.
int ValueInputCountOfReturn(Operator const* const op);

// Deoptimize bailout kind.
enum class DeoptimizeKind : uint8_t { kEager, kSoft };

Expand Down
27 changes: 27 additions & 0 deletions test/unittests/compiler/common-operator-reducer-unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,33 @@ TEST_F(CommonOperatorReducerTest, ReturnWithPhiAndEffectPhiAndMerge) {
IsReturn(vfalse, efalse, if_false)));
}

TEST_F(CommonOperatorReducerTest, MultiReturnWithPhiAndEffectPhiAndMerge) {
Node* cond = Parameter(2);
Node* branch = graph()->NewNode(common()->Branch(), cond, graph()->start());
Node* if_true = graph()->NewNode(common()->IfTrue(), branch);
Node* etrue = graph()->start();
Node* vtrue1 = Parameter(0);
Node* vtrue2 = Parameter(1);
Node* if_false = graph()->NewNode(common()->IfFalse(), branch);
Node* efalse = graph()->start();
Node* vfalse1 = Parameter(1);
Node* vfalse2 = Parameter(0);
Node* merge = graph()->NewNode(common()->Merge(2), if_true, if_false);
Node* ephi = graph()->NewNode(common()->EffectPhi(2), etrue, efalse, merge);
Node* phi1 = graph()->NewNode(
common()->Phi(MachineRepresentation::kTagged, 2), vtrue1, vfalse1, merge);
Node* phi2 = graph()->NewNode(
common()->Phi(MachineRepresentation::kTagged, 2), vtrue2, vfalse2, merge);

Node* zero = graph()->NewNode(common()->Int32Constant(0));
Node* ret =
graph()->NewNode(common()->Return(2), zero, phi1, phi2, ephi, merge);
graph()->SetEnd(graph()->NewNode(common()->End(1), ret));
StrictMock<MockAdvancedReducerEditor> editor;
Reduction const r = Reduce(&editor, ret);
// For now a return with multiple return values should not be reduced.
ASSERT_TRUE(!r.Changed());
}

// -----------------------------------------------------------------------------
// Select
Expand Down

0 comments on commit cfa6ce3

Please sign in to comment.