Skip to content

V
V8
Commit 767c34df authored by titzer's avatar titzer Committed by Commit bot
Browse files
Options

Revert of [turbofan] Take the immediate size in account when narrowing... 

Revert of [turbofan] Take the immediate size in account when narrowing ia32/x64 word comparison operators. (patchset #1 id:1 of https://codereview.chromium.org/1968453002/ )

Reason for revert:
Breaks a KCS demo:

BUG=chromium:611976

Original issue's description:
> [turbofan] Take the immediate size in account when narrowing ia32/x64 word comparison operators.
>
> Trying to re-land http://crrev.com/1948453002 after fixing assembler-x64.cc in http://crrev.com/1962563003.
>
> Before this patch, we would emit a cmp or test with a memory operand only if both of the operands in the IR were loads. Now if either of them is a load and the other one is an immediate, we can use a memory operand if the load representation machine size is wide enough to represent the latter.
>
> Committed: https://crrev.com/2da70f853d7f680d491c37c72d5ef04a85497ba9
> Cr-Commit-Position: refs/heads/master@{#36136}

TBR=bmeurer@chromium.org,epertoso@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.

Review-Url: https://codereview.chromium.org/1995303003
Cr-Commit-Position: refs/heads/master@{#36413}
parent 681ac897
Hide whitespace changes
Inline Side-by-side
Showing with 29 additions and 150 deletions
src/compiler/ia32/instruction-selector-ia32.cc
View file @ 767c34df
......@@ -1178,26 +1178,6 @@ void VisitCompare(InstructionSelector* selector, InstructionCode opcode,
VisitCompare(selector, opcode, g.UseRegister(left), g.Use(right), cont);
}
bool InferMachineRepresentation(Node* node,
MachineRepresentation* representation) {
if (node->opcode() == IrOpcode::kLoad) {
*representation = LoadRepresentationOf(node->op()).representation();
return true;
}
if (node->opcode() != IrOpcode::kInt32Constant) {
return false;
}
int32_t value = OpParameter<int32_t>(node->op());
if (is_int8(value)) {
*representation = MachineRepresentation::kWord8;
} else if (is_int16(value)) {
*representation = MachineRepresentation::kWord16;
} else {
*representation = MachineRepresentation::kWord32;
}
return true;
}
// Tries to match the size of the given opcode to that of the operands, if
// possible.
InstructionCode TryNarrowOpcodeSize(InstructionCode opcode, Node* left,
......@@ -1205,22 +1185,20 @@ InstructionCode TryNarrowOpcodeSize(InstructionCode opcode, Node* left,
if (opcode != kIA32Cmp && opcode != kIA32Test) {
return opcode;
}
// We only do this if at least one of the two operands is a load.
// TODO(epertoso): we can probably get some size information out of phi nodes.
if (left->opcode() != IrOpcode::kLoad && right->opcode() != IrOpcode::kLoad) {
return opcode;
}
MachineRepresentation left_representation, right_representation;
if (!InferMachineRepresentation(left, &left_representation) ||
!InferMachineRepresentation(right, &right_representation)) {
// Currently, if one of the two operands is not a Load, we don't know what its
// machine representation is, so we bail out.
// TODO(epertoso): we can probably get some size information out of immediates
// and phi nodes.
if (left->opcode() != IrOpcode::kLoad || right->opcode() != IrOpcode::kLoad) {
return opcode;
}
// If the representations don't match, both operands will be
// If the load representations don't match, both operands will be
// zero/sign-extended to 32bit.
if (left_representation != right_representation) {
LoadRepresentation left_representation = LoadRepresentationOf(left->op());
if (left_representation != LoadRepresentationOf(right->op())) {
return opcode;
}
switch (left_representation) {
switch (left_representation.representation()) {
case MachineRepresentation::kBit:
case MachineRepresentation::kWord8:
return opcode == kIA32Cmp ? kIA32Cmp8 : kIA32Test8;
......@@ -1268,33 +1246,10 @@ void VisitWordCompare(InstructionSelector* selector, Node* node,
// Match immediates on right side of comparison.
if (g.CanBeImmediate(right)) {
if (g.CanBeMemoryOperand(narrowed_opcode, node, left)) {
// If we're truncating the immediate (32 bits to 16 or 8), comparison
// semantics should take the signedness/unsignedness of the op into
// account.
if (narrowed_opcode != opcode &&
LoadRepresentationOf(left->op()).IsUnsigned()) {
switch (cont->condition()) {
case FlagsCondition::kSignedLessThan:
cont->OverwriteAndNegateIfEqual(FlagsCondition::kUnsignedLessThan);
break;
case FlagsCondition::kSignedGreaterThan:
cont->OverwriteAndNegateIfEqual(
FlagsCondition::kUnsignedGreaterThan);
break;
case FlagsCondition::kSignedLessThanOrEqual:
cont->OverwriteAndNegateIfEqual(
FlagsCondition::kUnsignedLessThanOrEqual);
break;
case FlagsCondition::kSignedGreaterThanOrEqual:
cont->OverwriteAndNegateIfEqual(
FlagsCondition::kUnsignedGreaterThanOrEqual);
break;
default:
break;
}
}
return VisitCompareWithMemoryOperand(selector, narrowed_opcode, left,
if (g.CanBeMemoryOperand(opcode, node, left)) {
// TODO(epertoso): we should use `narrowed_opcode' here once we match
// immediates too.
return VisitCompareWithMemoryOperand(selector, opcode, left,
g.UseImmediate(right), cont);
}
return VisitCompare(selector, opcode, g.Use(left), g.UseImmediate(right),
......
src/compiler/x64/instruction-selector-x64.cc
View file @ 767c34df
......@@ -1506,35 +1506,6 @@ void VisitCompare(InstructionSelector* selector, InstructionCode opcode,
VisitCompare(selector, opcode, g.UseRegister(left), g.Use(right), cont);
}
bool InferMachineRepresentation(Node* node,
MachineRepresentation* representation) {
if (node->opcode() == IrOpcode::kLoad) {
*representation = LoadRepresentationOf(node->op()).representation();
return true;
}
int64_t value = 0;
switch (node->opcode()) {
case IrOpcode::kInt32Constant:
value = OpParameter<int32_t>(node->op());
break;
case IrOpcode::kInt64Constant:
value = OpParameter<int64_t>(node->op());
break;
default:
return false;
}
if (is_int8(value)) {
*representation = MachineRepresentation::kWord8;
} else if (is_int16(value)) {
*representation = MachineRepresentation::kWord16;
} else if (is_int32(value)) {
*representation = MachineRepresentation::kWord32;
} else {
return false;
}
return true;
}
// Tries to match the size of the given opcode to that of the operands, if
// possible.
InstructionCode TryNarrowOpcodeSize(InstructionCode opcode, Node* left,
......@@ -1542,22 +1513,20 @@ InstructionCode TryNarrowOpcodeSize(InstructionCode opcode, Node* left,
if (opcode != kX64Cmp32 && opcode != kX64Test32) {
return opcode;
}
// We only do this if at least one of the two operands is a load.
// TODO(epertoso): we can probably get some size information out of phi nodes.
if (left->opcode() != IrOpcode::kLoad && right->opcode() != IrOpcode::kLoad) {
// Currently, if one of the two operands is not a Load, we don't know what its
// machine representation is, so we bail out.
// TODO(epertoso): we can probably get some size information out of immediates
// and phi nodes.
if (left->opcode() != IrOpcode::kLoad || right->opcode() != IrOpcode::kLoad) {
return opcode;
}
MachineRepresentation left_representation, right_representation;
if (!InferMachineRepresentation(left, &left_representation) ||
!InferMachineRepresentation(right, &right_representation)) {
return opcode;
}
// If the representations don't match, both operands will be
// If the load representations don't match, both operands will be
// zero/sign-extended to 32bit.
if (left_representation != right_representation) {
LoadRepresentation left_representation = LoadRepresentationOf(left->op());
if (left_representation != LoadRepresentationOf(right->op())) {
return opcode;
}
switch (left_representation) {
switch (left_representation.representation()) {
case MachineRepresentation::kBit:
case MachineRepresentation::kWord8:
return opcode == kX64Cmp32 ? kX64Cmp8 : kX64Test8;
......@@ -1575,46 +1544,21 @@ void VisitWordCompare(InstructionSelector* selector, Node* node,
Node* left = node->InputAt(0);
Node* right = node->InputAt(1);
InstructionCode narrowed_opcode = TryNarrowOpcodeSize(opcode, left, right);
opcode = TryNarrowOpcodeSize(opcode, left, right);
// If one of the two inputs is an immediate, make sure it's on the right, or
// if one of the two inputs is a memory operand, make sure it's on the left.
if ((!g.CanBeImmediate(right) && g.CanBeImmediate(left)) ||
(g.CanBeMemoryOperand(narrowed_opcode, node, right) &&
!g.CanBeMemoryOperand(narrowed_opcode, node, left))) {
(g.CanBeMemoryOperand(opcode, node, right) &&
!g.CanBeMemoryOperand(opcode, node, left))) {
if (!node->op()->HasProperty(Operator::kCommutative)) cont->Commute();
std::swap(left, right);
}
// Match immediates on right side of comparison.
if (g.CanBeImmediate(right)) {
if (g.CanBeMemoryOperand(narrowed_opcode, node, left)) {
// If we're truncating the immediate (32 bits to 16 or 8), comparison
// semantics should take the signedness/unsignedness of the op into
// account.
if (narrowed_opcode != opcode &&
LoadRepresentationOf(left->op()).IsUnsigned()) {
switch (cont->condition()) {
case FlagsCondition::kSignedLessThan:
cont->OverwriteAndNegateIfEqual(FlagsCondition::kUnsignedLessThan);
break;
case FlagsCondition::kSignedGreaterThan:
cont->OverwriteAndNegateIfEqual(
FlagsCondition::kUnsignedGreaterThan);
break;
case FlagsCondition::kSignedLessThanOrEqual:
cont->OverwriteAndNegateIfEqual(
FlagsCondition::kUnsignedLessThanOrEqual);
break;
case FlagsCondition::kSignedGreaterThanOrEqual:
cont->OverwriteAndNegateIfEqual(
FlagsCondition::kUnsignedGreaterThanOrEqual);
break;
default:
break;
}
}
return VisitCompareWithMemoryOperand(selector, narrowed_opcode, left,
if (g.CanBeMemoryOperand(opcode, node, left)) {
return VisitCompareWithMemoryOperand(selector, opcode, left,
g.UseImmediate(right), cont);
}
return VisitCompare(selector, opcode, g.Use(left), g.UseImmediate(right),
......@@ -1622,8 +1566,8 @@ void VisitWordCompare(InstructionSelector* selector, Node* node,
}
// Match memory operands on left side of comparison.
if (g.CanBeMemoryOperand(narrowed_opcode, node, left)) {
return VisitCompareWithMemoryOperand(selector, narrowed_opcode, left,
if (g.CanBeMemoryOperand(opcode, node, left)) {
return VisitCompareWithMemoryOperand(selector, opcode, left,
g.UseRegister(right), cont);
}
......
test/cctest/compiler/test-run-load-store.cc
View file @ 767c34df
......@@ -251,26 +251,6 @@ TEST(RunLoadStoreSignExtend32) {
}
}
TEST(RunLoadEliminationWithCompareAndTest) {
int8_t byte = 0x81;
int16_t word = 0xf00f;
RawMachineAssemblerTester<int32_t> m;
Node* load8 = m.LoadFromPointer(&byte, MachineType::Int8());
RawMachineLabel a, b, c, d;
m.Branch(m.Word32And(load8, m.Int32Constant(-0x80)), &b, &a);
m.Bind(&a);
m.Return(m.Int32Constant(0));
m.Bind(&b);
Node* load16 = m.LoadFromPointer(&word, MachineType::Int16());
m.Branch(m.Word32And(load16, m.Int32Constant(-0x7fff)), &d, &c);
m.Bind(&c);
m.Return(m.Int32Constant(0));
m.Bind(&d);
m.Return(m.Int32Constant(1));
CHECK_EQ(1, m.Call());
}
TEST(RunLoadStoreZeroExtend32) {
uint32_t buffer[4];
RawMachineAssemblerTester<uint32_t> m;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment