Commit 622852e5 authored by Benedikt Meurer's avatar Benedikt Meurer Committed by Commit Bot

[turbofan] Collect and use SignedSmall input feedback for Divide.

For Divide operations like

  r = a / b

where r has only truncated uses (i.e. only used in bitwise operations),
we used to generate a Float64Div unless we statically knew something
about a and b, even if a and b have always been integers so far.
Crankshaft was able to generate an integer division here, because
Fullcodegen collected feedback independently for inputs and outputs of
binary operations.

This adds new BinaryOperationFeedback::kSignedSmallInputs, which is used
specifically for Divide to state that we have seen only SignedSmall
inputs thus far, but the outputs weren't always in the SignedSmall
range.

The issue was discovered in a WebGL Triangulation library and reported
via https://twitter.com/mourner/status/895708603117518848 after Node
8.3.0 was released with I+TF.

R=jarin@chromium.org

Bug: v8:6698
Change-Id: I830e421a3bf91fc8fa3665cbb706bc13675a6d2b
Reviewed-on: https://chromium-review.googlesource.com/612063
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: 's avatarJaroslav Sevcik <jarin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47302}
parent f984eb1b
......@@ -599,6 +599,8 @@ struct JSOperatorGlobalCache final {
Name##Operator<BinaryOperationHint::kNone> k##Name##NoneOperator; \
Name##Operator<BinaryOperationHint::kSignedSmall> \
k##Name##SignedSmallOperator; \
Name##Operator<BinaryOperationHint::kSignedSmallInputs> \
k##Name##SignedSmallInputsOperator; \
Name##Operator<BinaryOperationHint::kSigned32> k##Name##Signed32Operator; \
Name##Operator<BinaryOperationHint::kNumber> k##Name##NumberOperator; \
Name##Operator<BinaryOperationHint::kNumberOrOddball> \
......@@ -652,6 +654,8 @@ CACHED_OP_LIST(CACHED_OP)
return &cache_.k##Name##NoneOperator; \
case BinaryOperationHint::kSignedSmall: \
return &cache_.k##Name##SignedSmallOperator; \
case BinaryOperationHint::kSignedSmallInputs: \
return &cache_.k##Name##SignedSmallInputsOperator; \
case BinaryOperationHint::kSigned32: \
return &cache_.k##Name##Signed32Operator; \
case BinaryOperationHint::kNumber: \
......
......@@ -22,6 +22,9 @@ bool BinaryOperationHintToNumberOperationHint(
case BinaryOperationHint::kSignedSmall:
*number_hint = NumberOperationHint::kSignedSmall;
return true;
case BinaryOperationHint::kSignedSmallInputs:
*number_hint = NumberOperationHint::kSignedSmallInputs;
return true;
case BinaryOperationHint::kSigned32:
*number_hint = NumberOperationHint::kSigned32;
return true;
......
......@@ -91,6 +91,7 @@ UseInfo CheckedUseInfoAsWord32FromHint(
IdentifyZeros identify_zeros = kDistinguishZeros) {
switch (hint) {
case NumberOperationHint::kSignedSmall:
case NumberOperationHint::kSignedSmallInputs:
return UseInfo::CheckedSignedSmallAsWord32(identify_zeros);
case NumberOperationHint::kSigned32:
return UseInfo::CheckedSigned32AsWord32(identify_zeros);
......@@ -105,6 +106,7 @@ UseInfo CheckedUseInfoAsWord32FromHint(
UseInfo CheckedUseInfoAsFloat64FromHint(NumberOperationHint hint) {
switch (hint) {
case NumberOperationHint::kSignedSmall:
case NumberOperationHint::kSignedSmallInputs:
case NumberOperationHint::kSigned32:
// Not used currently.
UNREACHABLE();
......@@ -1654,8 +1656,8 @@ class RepresentationSelector {
// Try to use type feedback.
NumberOperationHint hint = NumberOperationHintOf(node->op());
switch (hint) {
case NumberOperationHint::kSigned32:
case NumberOperationHint::kSignedSmall:
case NumberOperationHint::kSigned32: {
if (propagate()) {
VisitBinop(node, CheckedUseInfoAsWord32FromHint(hint),
MachineRepresentation::kBit);
......@@ -1679,7 +1681,9 @@ class RepresentationSelector {
}
}
return;
}
case NumberOperationHint::kSignedSmallInputs:
// This doesn't make sense for compare operations.
UNREACHABLE();
case NumberOperationHint::kNumberOrOddball:
// Abstract and strict equality don't perform ToNumber conversions
// on Oddballs, so make sure we don't accidentially sneak in a
......@@ -1850,19 +1854,21 @@ class RepresentationSelector {
}
}
if (hint == NumberOperationHint::kSignedSmall ||
hint == NumberOperationHint::kSigned32) {
if (hint == NumberOperationHint::kSigned32 ||
hint == NumberOperationHint::kSignedSmall ||
hint == NumberOperationHint::kSignedSmallInputs) {
// If the result is truncated, we only need to check the inputs.
if (truncation.IsUsedAsWord32()) {
VisitBinop(node, CheckedUseInfoAsWord32FromHint(hint),
MachineRepresentation::kWord32);
if (lower()) DeferReplacement(node, lowering->Int32Div(node));
} else {
return;
} else if (hint != NumberOperationHint::kSignedSmallInputs) {
VisitBinop(node, CheckedUseInfoAsWord32FromHint(hint),
MachineRepresentation::kWord32, Type::Signed32());
if (lower()) ChangeToInt32OverflowOp(node);
return;
}
return;
}
// default case => Float64Div
......@@ -2656,6 +2662,7 @@ class RepresentationSelector {
switch (hint) {
case NumberOperationHint::kSigned32:
case NumberOperationHint::kSignedSmall:
case NumberOperationHint::kSignedSmallInputs:
VisitUnop(node, CheckedUseInfoAsWord32FromHint(hint),
MachineRepresentation::kWord32, Type::Signed32());
break;
......
......@@ -401,6 +401,8 @@ std::ostream& operator<<(std::ostream& os, NumberOperationHint hint) {
switch (hint) {
case NumberOperationHint::kSignedSmall:
return os << "SignedSmall";
case NumberOperationHint::kSignedSmallInputs:
return os << "SignedSmallInputs";
case NumberOperationHint::kSigned32:
return os << "Signed32";
case NumberOperationHint::kNumber:
......@@ -780,6 +782,8 @@ struct SimplifiedOperatorGlobalCache final {
}; \
Name##Operator<NumberOperationHint::kSignedSmall> \
k##Name##SignedSmallOperator; \
Name##Operator<NumberOperationHint::kSignedSmallInputs> \
k##Name##SignedSmallInputsOperator; \
Name##Operator<NumberOperationHint::kSigned32> k##Name##Signed32Operator; \
Name##Operator<NumberOperationHint::kNumber> k##Name##NumberOperator; \
Name##Operator<NumberOperationHint::kNumberOrOddball> \
......@@ -940,6 +944,8 @@ const Operator* SimplifiedOperatorBuilder::SpeculativeToNumber(
switch (hint) {
case NumberOperationHint::kSignedSmall:
return &cache_.kSpeculativeToNumberSignedSmallOperator;
case NumberOperationHint::kSignedSmallInputs:
break;
case NumberOperationHint::kSigned32:
return &cache_.kSpeculativeToNumberSigned32Operator;
case NumberOperationHint::kNumber:
......@@ -1067,6 +1073,8 @@ const Operator* SimplifiedOperatorBuilder::StringFromCodePoint(
switch (hint) { \
case NumberOperationHint::kSignedSmall: \
return &cache_.k##Name##SignedSmallOperator; \
case NumberOperationHint::kSignedSmallInputs: \
return &cache_.k##Name##SignedSmallInputsOperator; \
case NumberOperationHint::kSigned32: \
return &cache_.k##Name##Signed32Operator; \
case NumberOperationHint::kNumber: \
......
......@@ -234,10 +234,11 @@ Handle<Map> FastMapParameterOf(const Operator* op);
// A hint for speculative number operations.
enum class NumberOperationHint : uint8_t {
kSignedSmall, // Inputs were always Smi so far, output was in Smi range.
kSigned32, // Inputs and output were Signed32 so far.
kNumber, // Inputs were Number, output was Number.
kNumberOrOddball, // Inputs were Number or Oddball, output was Number.
kSignedSmall, // Inputs were Smi, output was in Smi.
kSignedSmallInputs, // Inputs were Smi, output was Number.
kSigned32, // Inputs were Signed32, output was Number.
kNumber, // Inputs were Number, output was Number.
kNumberOrOddball, // Inputs were Number or Oddball, output was Number.
};
size_t hash_value(NumberOperationHint);
......
......@@ -181,6 +181,8 @@ BinaryOperationHint BinaryOperationHintFromFeedback(int type_feedback) {
return BinaryOperationHint::kNone;
case BinaryOperationFeedback::kSignedSmall:
return BinaryOperationHint::kSignedSmall;
case BinaryOperationFeedback::kSignedSmallInputs:
return BinaryOperationHint::kSignedSmallInputs;
case BinaryOperationFeedback::kNumber:
return BinaryOperationHint::kNumber;
case BinaryOperationFeedback::kNumberOrOddball:
......
......@@ -1254,8 +1254,8 @@ inline uint32_t ObjectHash(Address address) {
// Type feedback is encoded in such a way that, we can combine the feedback
// at different points by performing an 'OR' operation. Type feedback moves
// to a more generic type when we combine feedback.
// kSignedSmall -> kNumber -> kNumberOrOddball -> kAny
// kString -> kAny
// kSignedSmall -> kSignedSmallInputs -> kNumber -> kNumberOrOddball -> kAny
// kString -> kAny
// TODO(mythria): Remove kNumber type when crankshaft can handle Oddballs
// similar to Numbers. We don't need kNumber feedback for Turbofan. Extra
// information about Number might reduce few instructions but causes more
......@@ -1266,10 +1266,11 @@ class BinaryOperationFeedback {
enum {
kNone = 0x0,
kSignedSmall = 0x1,
kNumber = 0x3,
kNumberOrOddball = 0x7,
kString = 0x8,
kAny = 0x1F
kSignedSmallInputs = 0x3,
kNumber = 0x7,
kNumberOrOddball = 0xF,
kString = 0x10,
kAny = 0x3F
};
};
......
......@@ -463,7 +463,8 @@ Node* BinaryOpAssembler::Generate_DivideWithFeedback(
BIND(&bailout);
{
var_type_feedback->Bind(SmiConstant(BinaryOperationFeedback::kNumber));
var_type_feedback->Bind(
SmiConstant(BinaryOperationFeedback::kSignedSmallInputs));
Node* value = Float64Div(SmiToFloat64(lhs), SmiToFloat64(rhs));
var_result.Bind(AllocateHeapNumberWithValue(value));
Goto(&end);
......
......@@ -13,6 +13,8 @@ std::ostream& operator<<(std::ostream& os, BinaryOperationHint hint) {
return os << "None";
case BinaryOperationHint::kSignedSmall:
return os << "SignedSmall";
case BinaryOperationHint::kSignedSmallInputs:
return os << "SignedSmallInputs";
case BinaryOperationHint::kSigned32:
return os << "Signed32";
case BinaryOperationHint::kNumber:
......
......@@ -15,6 +15,7 @@ namespace internal {
enum class BinaryOperationHint : uint8_t {
kNone,
kSignedSmall,
kSignedSmallInputs,
kSigned32,
kNumber,
kNumberOrOddball,
......
......@@ -620,7 +620,7 @@ TEST(InterpreterBinaryOpTypeFeedback) {
BinaryOperationFeedback::kSignedSmall},
{Token::Value::DIV, ast_factory.NewSmi(3), ast_factory.NewSmi(2),
isolate->factory()->NewHeapNumber(3.0 / 2.0),
BinaryOperationFeedback::kNumber},
BinaryOperationFeedback::kSignedSmallInputs},
{Token::Value::DIV, ast_factory.NewNumber(3.1415), ast_factory.NewSmi(3),
isolate->factory()->NewHeapNumber(3.1415 / 3),
BinaryOperationFeedback::kNumber},
......
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