From: jkummerow@chromium.org Date: Wed, 19 Feb 2014 10:30:39 +0000 (+0000) Subject: Fix Hydrogen bounds check elimination X-Git-Tag: upstream/4.7.83~10623 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=6e3b81a7b2823e925b1d9115060d403239662b36;p=platform%2Fupstream%2Fv8.git Fix Hydrogen bounds check elimination When combining bounds checks, they must all be moved before the first load/store that they are guarding. BUG=chromium:344186 LOG=y R=svenpanne@chromium.org Review URL: https://codereview.chromium.org/172093002 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@19475 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- diff --git a/src/hydrogen-bce.cc b/src/hydrogen-bce.cc index 869db54..7e9a556 100644 --- a/src/hydrogen-bce.cc +++ b/src/hydrogen-bce.cc @@ -91,8 +91,8 @@ class BoundsCheckKey : public ZoneObject { private: BoundsCheckKey(HValue* index_base, HValue* length) - : index_base_(index_base), - length_(length) { } + : index_base_(index_base), + length_(length) { } HValue* index_base_; HValue* length_; @@ -144,10 +144,7 @@ class BoundsCheckBbData: public ZoneObject { // (either upper or lower; note that HasSingleCheck() becomes false). // Otherwise one of the current checks is modified so that it also covers // new_offset, and new_check is removed. - // - // If the check cannot be modified because the context is unknown it - // returns false, otherwise it returns true. - bool CoverCheck(HBoundsCheck* new_check, + void CoverCheck(HBoundsCheck* new_check, int32_t new_offset) { ASSERT(new_check->index()->representation().IsSmiOrInteger32()); bool keep_new_check = false; @@ -158,15 +155,7 @@ class BoundsCheckBbData: public ZoneObject { keep_new_check = true; upper_check_ = new_check; } else { - bool result = BuildOffsetAdd(upper_check_, - &added_upper_index_, - &added_upper_offset_, - Key()->IndexBase(), - new_check->index()->representation(), - new_offset); - if (!result) return false; - upper_check_->ReplaceAllUsesWith(upper_check_->index()); - upper_check_->SetOperandAt(0, added_upper_index_); + TightenCheck(upper_check_, new_check); } } else if (new_offset < lower_offset_) { lower_offset_ = new_offset; @@ -174,32 +163,27 @@ class BoundsCheckBbData: public ZoneObject { keep_new_check = true; lower_check_ = new_check; } else { - bool result = BuildOffsetAdd(lower_check_, - &added_lower_index_, - &added_lower_offset_, - Key()->IndexBase(), - new_check->index()->representation(), - new_offset); - if (!result) return false; - lower_check_->ReplaceAllUsesWith(lower_check_->index()); - lower_check_->SetOperandAt(0, added_lower_index_); + TightenCheck(lower_check_, new_check); } } else { - ASSERT(false); + // Should never have called CoverCheck() in this case. + UNREACHABLE(); } if (!keep_new_check) { new_check->block()->graph()->isolate()->counters()-> bounds_checks_eliminated()->Increment(); new_check->DeleteAndReplaceWith(new_check->ActualValue()); + } else { + HBoundsCheck* first_check = new_check == lower_check_ ? upper_check_ + : lower_check_; + // The length is guaranteed to be live at first_check. + ASSERT(new_check->length() == first_check->length()); + HInstruction* old_position = new_check->next(); + new_check->Unlink(); + new_check->InsertAfter(first_check); + MoveIndexIfNecessary(new_check->index(), new_check, old_position); } - - return true; - } - - void RemoveZeroOperations() { - RemoveZeroAdd(&added_lower_index_, &added_lower_offset_); - RemoveZeroAdd(&added_upper_index_, &added_upper_offset_); } BoundsCheckBbData(BoundsCheckKey* key, @@ -210,18 +194,14 @@ class BoundsCheckBbData: public ZoneObject { HBoundsCheck* upper_check, BoundsCheckBbData* next_in_bb, BoundsCheckBbData* father_in_dt) - : key_(key), - lower_offset_(lower_offset), - upper_offset_(upper_offset), - basic_block_(bb), - lower_check_(lower_check), - upper_check_(upper_check), - added_lower_index_(NULL), - added_lower_offset_(NULL), - added_upper_index_(NULL), - added_upper_offset_(NULL), - next_in_bb_(next_in_bb), - father_in_dt_(father_in_dt) { } + : key_(key), + lower_offset_(lower_offset), + upper_offset_(upper_offset), + basic_block_(bb), + lower_check_(lower_check), + upper_check_(upper_check), + next_in_bb_(next_in_bb), + father_in_dt_(father_in_dt) { } private: BoundsCheckKey* key_; @@ -230,57 +210,52 @@ class BoundsCheckBbData: public ZoneObject { HBasicBlock* basic_block_; HBoundsCheck* lower_check_; HBoundsCheck* upper_check_; - HInstruction* added_lower_index_; - HConstant* added_lower_offset_; - HInstruction* added_upper_index_; - HConstant* added_upper_offset_; BoundsCheckBbData* next_in_bb_; BoundsCheckBbData* father_in_dt_; - // Given an existing add instruction and a bounds check it tries to - // find the current context (either of the add or of the check index). - HValue* IndexContext(HInstruction* add, HBoundsCheck* check) { - if (add != NULL && add->IsAdd()) { - return HAdd::cast(add)->context(); + void MoveIndexIfNecessary(HValue* index_raw, + HBoundsCheck* insert_before, + HInstruction* end_of_scan_range) { + ASSERT(index_raw->IsAdd() || index_raw->IsSub()); + HArithmeticBinaryOperation* index = + HArithmeticBinaryOperation::cast(index_raw); + HValue* left_input = index->left(); + HValue* right_input = index->right(); + bool must_move_index = false; + bool must_move_left_input = false; + bool must_move_right_input = false; + for (HInstruction* cursor = end_of_scan_range; cursor != insert_before;) { + if (cursor == left_input) must_move_left_input = true; + if (cursor == right_input) must_move_right_input = true; + if (cursor == index) must_move_index = true; + if (cursor->previous() == NULL) { + cursor = cursor->block()->dominator()->end(); + } else { + cursor = cursor->previous(); + } } - if (check->index()->IsBinaryOperation()) { - return HBinaryOperation::cast(check->index())->context(); + if (must_move_index) { + index->Unlink(); + index->InsertBefore(insert_before); } - return NULL; - } - - // This function returns false if it cannot build the add because the - // current context cannot be determined. - bool BuildOffsetAdd(HBoundsCheck* check, - HInstruction** add, - HConstant** constant, - HValue* original_value, - Representation representation, - int32_t new_offset) { - HValue* index_context = IndexContext(*add, check); - if (index_context == NULL) return false; - - Zone* zone = BasicBlock()->zone(); - HConstant* new_constant = HConstant::New(zone, index_context, - new_offset, representation); - if (*add == NULL) { - new_constant->InsertBefore(check); - (*add) = HAdd::New(zone, index_context, original_value, new_constant); - (*add)->AssumeRepresentation(representation); - (*add)->InsertBefore(check); - } else { - new_constant->InsertBefore(*add); - (*constant)->DeleteAndReplaceWith(new_constant); + // The BCE algorithm only selects mergeable bounds checks that share + // the same "index_base", so we'll only ever have to move constants. + if (must_move_left_input) { + HConstant::cast(left_input)->Unlink(); + HConstant::cast(left_input)->InsertBefore(index); + } + if (must_move_right_input) { + HConstant::cast(right_input)->Unlink(); + HConstant::cast(right_input)->InsertBefore(index); } - *constant = new_constant; - return true; } - void RemoveZeroAdd(HInstruction** add, HConstant** constant) { - if (*add != NULL && (*add)->IsAdd() && (*constant)->Integer32Value() == 0) { - (*add)->DeleteAndReplaceWith(HAdd::cast(*add)->left()); - (*constant)->DeleteAndReplaceWith(NULL); - } + void TightenCheck(HBoundsCheck* original_check, + HBoundsCheck* tighter_check) { + ASSERT(original_check->length() == tighter_check->length()); + MoveIndexIfNecessary(tighter_check->index(), original_check, tighter_check); + original_check->ReplaceAllUsesWith(original_check->index()); + original_check->SetOperandAt(0, tighter_check->index()); } DISALLOW_COPY_AND_ASSIGN(BoundsCheckBbData); @@ -394,11 +369,9 @@ BoundsCheckBbData* HBoundsCheckEliminationPhase::PreProcessBlock( bb->graph()->isolate()->counters()-> bounds_checks_eliminated()->Increment(); check->DeleteAndReplaceWith(check->ActualValue()); - } else if (data->BasicBlock() != bb || - !data->CoverCheck(check, offset)) { - // If the check is in the current BB we try to modify it by calling - // "CoverCheck", but if also that fails we record the current offsets - // in a new data instance because from now on they are covered. + } else if (data->BasicBlock() == bb) { + data->CoverCheck(check, offset); + } else { int32_t new_lower_offset = offset < data->LowerOffset() ? offset : data->LowerOffset(); @@ -424,7 +397,6 @@ BoundsCheckBbData* HBoundsCheckEliminationPhase::PreProcessBlock( void HBoundsCheckEliminationPhase::PostProcessBlock( HBasicBlock* block, BoundsCheckBbData* data) { while (data != NULL) { - data->RemoveZeroOperations(); if (data->FatherInDominatorTree()) { table_.Insert(data->Key(), data->FatherInDominatorTree(), zone()); } else { diff --git a/test/mjsunit/regress/regress-crbug-344186.js b/test/mjsunit/regress/regress-crbug-344186.js new file mode 100644 index 0000000..6486f38 --- /dev/null +++ b/test/mjsunit/regress/regress-crbug-344186.js @@ -0,0 +1,20 @@ +// Copyright 2014 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --allow-natives-syntax + +var dummy = new Int32Array(100); +var array = new Int32Array(128); +function fun(base) { + array[base - 95] = 1; + array[base - 99] = 2; + array[base + 4] = 3; +} +fun(100); +%OptimizeFunctionOnNextCall(fun); +fun(0); + +for (var i = 0; i < dummy.length; i++) { + assertEquals(0, dummy[i]); +}