Commit d504203e authored by Benedikt Meurer's avatar Benedikt Meurer Committed by Commit Bot

[turbofan] Consistently use String feedback for JSAdd.

Currently we didn't always consistently use the String feedback on
JSAdd, but only if JSTypedLowering would already figure out statically
that one of the inputs is already a String. That leads to some odd
performance cliffs, as highlighted in the referenced bug.

This CL fixes the JSTypedLowering::ReduceJSAdd to always bake in the
String feedback. This improves the relevant performance tests from the
bug from

  console.timeEnd: Runtime join3, 967.512000
  console.timeEnd: Runtime join, 1004.599000
  console.timeEnd: Runtime join3, 1124.764000
  console.timeEnd: Runtime join, 966.164000
  console.timeEnd: Runtime join3, 1145.296000
  console.timeEnd: Runtime join, 966.176000
  console.timeEnd: Runtime join3, 1145.272000
  console.timeEnd: Runtime join, 931.266000

to

  console.timeEnd: Runtime join3, 903.050000
  console.timeEnd: Runtime join, 856.509000
  console.timeEnd: Runtime join3, 945.144000
  console.timeEnd: Runtime join, 840.038000
  console.timeEnd: Runtime join3, 927.965000
  console.timeEnd: Runtime join, 841.263000
  console.timeEnd: Runtime join3, 929.342000
  console.timeEnd: Runtime join, 858.143000

which corresponds to an 8-18% improvement.

Bug: v8:7415
Change-Id: I62e008298e4ee0864885b37817c91d055acf2a09
Reviewed-on: https://chromium-review.googlesource.com/936643Reviewed-by: 's avatarJaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51551}
parent 80e0a759
......@@ -504,6 +504,13 @@ Reduction JSTypedLowering::ReduceJSAdd(Node* node) {
r.ConvertInputsToNumber();
return r.ChangeToPureOperator(simplified()->NumberAdd(), Type::Number());
}
if (BinaryOperationHintOf(node->op()) == BinaryOperationHint::kString) {
// Always bake in String feedback into the graph.
// TODO(bmeurer): Consider adding a SpeculativeStringAdd operator,
// and use that in JSTypeHintLowering instead of looking at the
// binary operation feedback here.
r.CheckInputsToString();
}
if (r.OneInputIs(Type::String())) {
// We know that (at least) one input is already a String,
// so try to strength-reduce the non-String input.
......@@ -539,7 +546,7 @@ Reduction JSTypedLowering::ReduceJSAdd(Node* node) {
return ReduceCreateConsString(node);
}
// Eliminate useless concatenation of empty string.
if (BinaryOperationHintOf(node->op()) == BinaryOperationHint::kString) {
if (r.BothInputsAre(Type::String())) {
Node* effect = NodeProperties::GetEffectInput(node);
Node* control = NodeProperties::GetControlInput(node);
if (r.LeftInputIs(empty_string_type_)) {
......@@ -582,6 +589,8 @@ Reduction JSTypedLowering::ReduceJSAdd(Node* node) {
NodeProperties::ChangeOp(node, common()->Call(call_descriptor));
return Changed(node);
}
// We never get here when we had String feedback.
DCHECK_NE(BinaryOperationHint::kString, BinaryOperationHintOf(node->op()));
return NoChange();
}
......
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