From c981914d4c5c68178e7defb9b03bfb7926b447a3 Mon Sep 17 00:00:00 2001 From: "bmeurer@chromium.org" Date: Mon, 10 Mar 2014 05:52:03 +0000 Subject: [PATCH] Replace the recursion in PropagateMinusZeroChecks() with a loop and a worklist. Also refactor the related code in preparation for fixing the range analysis. BUG=v8:3204 LOG=y R=svenpanne@chromium.org Review URL: https://codereview.chromium.org/190713002 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@19737 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/hydrogen-instructions.cc | 93 ------------------------------------ src/hydrogen-instructions.h | 41 ---------------- src/hydrogen-minus-zero.cc | 91 ++++++++++++++++++++++++++--------- src/hydrogen-minus-zero.h | 12 ++++- 4 files changed, 78 insertions(+), 159 deletions(-) diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc index 7b4f758998..fb19209a25 100644 --- a/src/hydrogen-instructions.cc +++ b/src/hydrogen-instructions.cc @@ -3703,99 +3703,6 @@ void HAllocate::PrintDataTo(StringStream* stream) { } -HValue* HUnaryMathOperation::EnsureAndPropagateNotMinusZero( - BitVector* visited) { - visited->Add(id()); - if (representation().IsSmiOrInteger32() && - !value()->representation().Equals(representation())) { - if (value()->range() == NULL || value()->range()->CanBeMinusZero()) { - SetFlag(kBailoutOnMinusZero); - } - } - if (RequiredInputRepresentation(0).IsSmiOrInteger32() && - representation().Equals(RequiredInputRepresentation(0))) { - return value(); - } - return NULL; -} - - -HValue* HChange::EnsureAndPropagateNotMinusZero(BitVector* visited) { - visited->Add(id()); - if (from().IsSmiOrInteger32()) return NULL; - if (CanTruncateToInt32()) return NULL; - if (value()->range() == NULL || value()->range()->CanBeMinusZero()) { - SetFlag(kBailoutOnMinusZero); - } - ASSERT(!from().IsSmiOrInteger32() || !to().IsSmiOrInteger32()); - return NULL; -} - - -HValue* HForceRepresentation::EnsureAndPropagateNotMinusZero( - BitVector* visited) { - visited->Add(id()); - return value(); -} - - -HValue* HMod::EnsureAndPropagateNotMinusZero(BitVector* visited) { - visited->Add(id()); - if (range() == NULL || range()->CanBeMinusZero()) { - SetFlag(kBailoutOnMinusZero); - return left(); - } - return NULL; -} - - -HValue* HDiv::EnsureAndPropagateNotMinusZero(BitVector* visited) { - visited->Add(id()); - if (range() == NULL || range()->CanBeMinusZero()) { - SetFlag(kBailoutOnMinusZero); - } - return NULL; -} - - -HValue* HMathFloorOfDiv::EnsureAndPropagateNotMinusZero(BitVector* visited) { - visited->Add(id()); - SetFlag(kBailoutOnMinusZero); - return NULL; -} - - -HValue* HMul::EnsureAndPropagateNotMinusZero(BitVector* visited) { - visited->Add(id()); - if (range() == NULL || range()->CanBeMinusZero()) { - SetFlag(kBailoutOnMinusZero); - } - return NULL; -} - - -HValue* HSub::EnsureAndPropagateNotMinusZero(BitVector* visited) { - visited->Add(id()); - // Propagate to the left argument. If the left argument cannot be -0, then - // the result of the add operation cannot be either. - if (range() == NULL || range()->CanBeMinusZero()) { - return left(); - } - return NULL; -} - - -HValue* HAdd::EnsureAndPropagateNotMinusZero(BitVector* visited) { - visited->Add(id()); - // Propagate to the left argument. If the left argument cannot be -0, then - // the result of the sub operation cannot be either. - if (range() == NULL || range()->CanBeMinusZero()) { - return left(); - } - return NULL; -} - - bool HStoreKeyed::NeedsCanonicalization() { // If value is an integer or smi or comes from the result of a keyed load or // constant then it is either be a non-hole value or in the case of a constant diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h index 4c54c1a334..36ef717c5d 100644 --- a/src/hydrogen-instructions.h +++ b/src/hydrogen-instructions.h @@ -739,21 +739,6 @@ class HValue : public ZoneObject { return representation_.IsHeapObject() || type_.IsHeapObject(); } - // An operation needs to override this function iff: - // 1) it can produce an int32 output. - // 2) the true value of its output can potentially be minus zero. - // The implementation must set a flag so that it bails out in the case where - // it would otherwise output what should be a minus zero as an int32 zero. - // If the operation also exists in a form that takes int32 and outputs int32 - // then the operation should return its input value so that we can propagate - // back. There are three operations that need to propagate back to more than - // one input. They are phi and binary div and mul. They always return NULL - // and expect the caller to take care of things. - virtual HValue* EnsureAndPropagateNotMinusZero(BitVector* visited) { - visited->Add(id()); - return NULL; - } - // There are HInstructions that do not really change a value, they // only add pieces of information to it (like bounds checks, map checks, // smi checks...). @@ -1719,9 +1704,6 @@ class HForceRepresentation V8_FINAL : public HTemplateInstruction<1> { HValue* value() { return OperandAt(0); } - virtual HValue* EnsureAndPropagateNotMinusZero( - BitVector* visited) V8_OVERRIDE; - virtual Representation RequiredInputRepresentation(int index) V8_OVERRIDE { return representation(); // Same as the output representation. } @@ -1767,8 +1749,6 @@ class HChange V8_FINAL : public HUnaryOperation { return CheckUsesForFlag(kAllowUndefinedAsNaN); } - virtual HValue* EnsureAndPropagateNotMinusZero( - BitVector* visited) V8_OVERRIDE; virtual HType CalculateInferredType() V8_OVERRIDE; virtual HValue* Canonicalize() V8_OVERRIDE; @@ -2631,9 +2611,6 @@ class HUnaryMathOperation V8_FINAL : public HTemplateInstruction<2> { virtual void PrintDataTo(StringStream* stream) V8_OVERRIDE; - virtual HValue* EnsureAndPropagateNotMinusZero( - BitVector* visited) V8_OVERRIDE; - virtual Representation RequiredInputRepresentation(int index) V8_OVERRIDE { if (index == 0) { return Representation::Tagged(); @@ -4111,9 +4088,6 @@ class HMathFloorOfDiv V8_FINAL : public HBinaryOperation { HValue*, HValue*); - virtual HValue* EnsureAndPropagateNotMinusZero( - BitVector* visited) V8_OVERRIDE; - DECLARE_CONCRETE_INSTRUCTION(MathFloorOfDiv) protected: @@ -4714,9 +4688,6 @@ class HAdd V8_FINAL : public HArithmeticBinaryOperation { return !representation().IsTagged() && !representation().IsExternal(); } - virtual HValue* EnsureAndPropagateNotMinusZero( - BitVector* visited) V8_OVERRIDE; - virtual HValue* Canonicalize() V8_OVERRIDE; virtual bool TryDecompose(DecompositionResult* decomposition) V8_OVERRIDE { @@ -4773,9 +4744,6 @@ class HSub V8_FINAL : public HArithmeticBinaryOperation { HValue* left, HValue* right); - virtual HValue* EnsureAndPropagateNotMinusZero( - BitVector* visited) V8_OVERRIDE; - virtual HValue* Canonicalize() V8_OVERRIDE; virtual bool TryDecompose(DecompositionResult* decomposition) V8_OVERRIDE { @@ -4822,9 +4790,6 @@ class HMul V8_FINAL : public HArithmeticBinaryOperation { return mul; } - virtual HValue* EnsureAndPropagateNotMinusZero( - BitVector* visited) V8_OVERRIDE; - virtual HValue* Canonicalize() V8_OVERRIDE; // Only commutative if it is certain that not two objects are multiplicated. @@ -4862,9 +4827,6 @@ class HMod V8_FINAL : public HArithmeticBinaryOperation { HValue* left, HValue* right); - virtual HValue* EnsureAndPropagateNotMinusZero( - BitVector* visited) V8_OVERRIDE; - virtual HValue* Canonicalize() V8_OVERRIDE; virtual void UpdateRepresentation(Representation new_rep, @@ -4898,9 +4860,6 @@ class HDiv V8_FINAL : public HArithmeticBinaryOperation { HValue* left, HValue* right); - virtual HValue* EnsureAndPropagateNotMinusZero( - BitVector* visited) V8_OVERRIDE; - virtual HValue* Canonicalize() V8_OVERRIDE; virtual void UpdateRepresentation(Representation new_rep, diff --git a/src/hydrogen-minus-zero.cc b/src/hydrogen-minus-zero.cc index 316e0f5077..b1b1ed5eea 100644 --- a/src/hydrogen-minus-zero.cc +++ b/src/hydrogen-minus-zero.cc @@ -45,17 +45,13 @@ void HComputeMinusZeroChecksPhase::Run() { ASSERT(change->to().IsTagged() || change->to().IsDouble() || change->to().IsSmiOrInteger32()); - ASSERT(visited_.IsEmpty()); PropagateMinusZeroChecks(change->value()); - visited_.Clear(); } } else if (current->IsCompareMinusZeroAndBranch()) { HCompareMinusZeroAndBranch* check = HCompareMinusZeroAndBranch::cast(current); if (check->value()->representation().IsSmiOrInteger32()) { - ASSERT(visited_.IsEmpty()); PropagateMinusZeroChecks(check->value()); - visited_.Clear(); } } } @@ -64,28 +60,77 @@ void HComputeMinusZeroChecksPhase::Run() { void HComputeMinusZeroChecksPhase::PropagateMinusZeroChecks(HValue* value) { - for (HValue* current = value; - current != NULL && !visited_.Contains(current->id()); - current = current->EnsureAndPropagateNotMinusZero(&visited_)) { - // For phis, we must propagate the check to all of its inputs. - if (current->IsPhi()) { - visited_.Add(current->id()); - HPhi* phi = HPhi::cast(current); - for (int i = 0; i < phi->OperandCount(); ++i) { - PropagateMinusZeroChecks(phi->OperandAt(i)); - } - break; - } + ASSERT(worklist_.is_empty()); + ASSERT(in_worklist_.IsEmpty()); - // For multiplication, division, and Math.min/max(), we must propagate - // to the left and the right side. - if (current->IsMul() || current->IsDiv() || current->IsMathMinMax()) { - HBinaryOperation* operation = HBinaryOperation::cast(current); - operation->EnsureAndPropagateNotMinusZero(&visited_); - PropagateMinusZeroChecks(operation->left()); - PropagateMinusZeroChecks(operation->right()); + AddToWorklist(value); + while (!worklist_.is_empty()) { + value = worklist_.RemoveLast(); + + if (value->IsPhi()) { + // For phis, we must propagate the check to all of its inputs. + HPhi* phi = HPhi::cast(value); + for (int i = 0; i < phi->OperandCount(); ++i) { + AddToWorklist(phi->OperandAt(i)); + } + } else if (value->IsUnaryMathOperation()) { + HUnaryMathOperation* instr = HUnaryMathOperation::cast(value); + if (instr->representation().IsSmiOrInteger32() && + !instr->value()->representation().Equals(instr->representation())) { + if (instr->value()->range() == NULL || + instr->value()->range()->CanBeMinusZero()) { + instr->SetFlag(HValue::kBailoutOnMinusZero); + } + } + if (instr->RequiredInputRepresentation(0).IsSmiOrInteger32() && + instr->representation().Equals( + instr->RequiredInputRepresentation(0))) { + AddToWorklist(instr->value()); + } + } else if (value->IsChange()) { + HChange* instr = HChange::cast(value); + if (!instr->from().IsSmiOrInteger32() && + !instr->CanTruncateToInt32() && + (instr->value()->range() == NULL || + instr->value()->range()->CanBeMinusZero())) { + instr->SetFlag(HValue::kBailoutOnMinusZero); + } + } else if (value->IsForceRepresentation()) { + HForceRepresentation* instr = HForceRepresentation::cast(value); + AddToWorklist(instr->value()); + } else if (value->IsMod()) { + HMod* instr = HMod::cast(value); + if (instr->range() == NULL || instr->range()->CanBeMinusZero()) { + instr->SetFlag(HValue::kBailoutOnMinusZero); + AddToWorklist(instr->left()); + } + } else if (value->IsDiv() || value->IsMul()) { + HBinaryOperation* instr = HBinaryOperation::cast(value); + if (instr->range() == NULL || instr->range()->CanBeMinusZero()) { + instr->SetFlag(HValue::kBailoutOnMinusZero); + } + AddToWorklist(instr->right()); + AddToWorklist(instr->left()); + } else if (value->IsMathFloorOfDiv()) { + HMathFloorOfDiv* instr = HMathFloorOfDiv::cast(value); + instr->SetFlag(HValue::kBailoutOnMinusZero); + } else if (value->IsAdd() || value->IsSub()) { + HBinaryOperation* instr = HBinaryOperation::cast(value); + if (instr->range() == NULL || instr->range()->CanBeMinusZero()) { + // Propagate to the left argument. If the left argument cannot be -0, + // then the result of the add/sub operation cannot be either. + AddToWorklist(instr->left()); + } + } else if (value->IsMathMinMax()) { + HMathMinMax* instr = HMathMinMax::cast(value); + AddToWorklist(instr->right()); + AddToWorklist(instr->left()); } } + + in_worklist_.Clear(); + ASSERT(in_worklist_.IsEmpty()); + ASSERT(worklist_.is_empty()); } } } // namespace v8::internal diff --git a/src/hydrogen-minus-zero.h b/src/hydrogen-minus-zero.h index d23ec1196b..7b74ab99a4 100644 --- a/src/hydrogen-minus-zero.h +++ b/src/hydrogen-minus-zero.h @@ -38,14 +38,22 @@ class HComputeMinusZeroChecksPhase : public HPhase { public: explicit HComputeMinusZeroChecksPhase(HGraph* graph) : HPhase("H_Compute minus zero checks", graph), - visited_(graph->GetMaximumValueID(), zone()) { } + in_worklist_(graph->GetMaximumValueID(), zone()), + worklist_(32, zone()) {} void Run(); private: + void AddToWorklist(HValue* value) { + if (value->CheckFlag(HValue::kBailoutOnMinusZero)) return; + if (in_worklist_.Contains(value->id())) return; + in_worklist_.Add(value->id()); + worklist_.Add(value, zone()); + } void PropagateMinusZeroChecks(HValue* value); - BitVector visited_; + BitVector in_worklist_; + ZoneList worklist_; DISALLOW_COPY_AND_ASSIGN(HComputeMinusZeroChecksPhase); };