Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10032029:
Eliminate redundant array bound checks (checks already performed earlier in the DT). (Closed)
|
Created:
8 years, 8 months ago by Massi Modified:
8 years, 8 months ago CC:
v8-dev Visibility:
Public. |
DescriptionEliminate redundant array bound checks (checks already performed earlier in the DT).
As a special case, for checks on index expressions with the form (expr + constant) if a smaller constant is checked later in the DT also eliminate the check.
Finally, if a larger constant is checked later in the same BB do the more general check (larger constant) earlier instead of the less general one.
This will not cause useless deoptimizations because, since we are in the same BB, all the checks would have been executed anyway.
BUG=
TEST=
Committed: https://code.google.com/p/v8/source/detail?r=11437
Patch Set 1 #
Total comments: 14
Patch Set 2 : 'Addressed comments and fixed bugs.' #
Total comments: 29
Patch Set 3 : Fixed remaining implementation issues. #Patch Set 4 : Small code style fix. #
Total comments: 12
Patch Set 5 : Fixed style issues and shortened the phase name to help Golem understand it. #
Total comments: 11
Patch Set 6 : Changed implementation to handle negative values properly, and added tests. #Patch Set 7 : Fixed issue with lower bound changing in unusual ways. #
Total comments: 20
Patch Set 8 : Style fixes. #Patch Set 9 : More small style fixes. #
Total comments: 3
Messages
Total messages: 18 (0 generated)
First round of comments... http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2593 src/hydrogen.cc:2593: static BoundsCheckKey* CreateBoundsCheckKey(Zone* zone, To quote memegen: Just drop the "BoundsCheckKey" part in the function name, it's cleaner... ;-) http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2597 src/hydrogen.cc:2597: if (check->index()->IsAdd()) { It might be worthwhile to reduce the redundancy and nesting here somehow, a human brain's stack depth is very limited. :-) http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2690 src/hydrogen.cc:2690: BoundsCheckBbData* father_in_dt_; I think using "bb" and "dt" is a little bit cryptic, and we normally don't use the abbreviations. "bb" e.g. is already used for "block buffer", etc. Names which are blindingly obviously while writing the code aren't necessarily like this 2 weeks later. ;-) http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2693 src/hydrogen.cc:2693: bool BoundsCheckKeyMatch(void* key1, void* key2) { This should probably be "static". http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2701 src/hydrogen.cc:2701: void EliminateRedundantChecks(HBasicBlock* bb) { I don't think this function really belongs into this class. BoundsCheckTable's responsibility should just be a more sane^H^H^H cleanly typed interface to ZoneHashMap. http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2704 src/hydrogen.cc:2704: for (HInstruction* i = bb->first(); i != NULL; i = i->next()) { Perhaps the first loop can be extracted into a separate function? http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2775 src/hydrogen.cc:2775: map = new ZoneHashMap(BoundsCheckKeyMatch); Using an initializer is a nicer and more standard way for doing this.
I fixed the issues described in the comments. I also did the following: - Do what "ReplaceCheckedValues" did inside the new "EliminateRedundantChecks". - Do not move the index when we move a check, instead recreate it just before the "target" check. - Also add a HSimulate instruction to the index copy. - In BoundsCheckBbData, added fields to remember created indexes and their HSimulate. http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2593 src/hydrogen.cc:2593: static BoundsCheckKey* CreateBoundsCheckKey(Zone* zone, On 2012/04/12 09:45:42, Sven Panne wrote: > To quote memegen: Just drop the "BoundsCheckKey" part in the function name, it's > cleaner... ;-) Done. http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2597 src/hydrogen.cc:2597: if (check->index()->IsAdd()) { On 2012/04/12 09:45:42, Sven Panne wrote: > It might be worthwhile to reduce the redundancy and nesting here somehow, a > human brain's stack depth is very limited. :-) Done. http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2690 src/hydrogen.cc:2690: BoundsCheckBbData* father_in_dt_; On 2012/04/12 09:45:42, Sven Panne wrote: > I think using "bb" and "dt" is a little bit cryptic, and we normally don't use > the abbreviations. "bb" e.g. is already used for "block buffer", etc. Names > which are blindingly obviously while writing the code aren't necessarily like > this 2 weeks later. ;-) Done. http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2693 src/hydrogen.cc:2693: bool BoundsCheckKeyMatch(void* key1, void* key2) { On 2012/04/12 09:45:42, Sven Panne wrote: > This should probably be "static". Done. http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2701 src/hydrogen.cc:2701: void EliminateRedundantChecks(HBasicBlock* bb) { On 2012/04/12 09:45:42, Sven Panne wrote: > I don't think this function really belongs into this class. BoundsCheckTable's > responsibility should just be a more sane^H^H^H cleanly typed interface to > ZoneHashMap. Done. http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2704 src/hydrogen.cc:2704: for (HInstruction* i = bb->first(); i != NULL; i = i->next()) { On 2012/04/12 09:45:42, Sven Panne wrote: > Perhaps the first loop can be extracted into a separate function? Done. http://codereview.chromium.org/10032029/diff/1/src/hydrogen.cc#newcode2775 src/hydrogen.cc:2775: map = new ZoneHashMap(BoundsCheckKeyMatch); On 2012/04/12 09:45:42, Sven Panne wrote: > Using an initializer is a nicer and more standard way for doing this. Done.
Next round of comments... http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2592 src/hydrogen.cc:2592: if (check->index()->IsAdd()) { If we extract a function like 'MatchOffsetExpresssion(HValue** index_base, HConstant** constant)' and massage CoverCheck a tiny bit, it should be straightforward to handle the HMinus case, too. I leave it up to you to do this now or in a separate CL. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2637 src/hydrogen.cc:2637: // class BoundsCheckBbData { Remove that line. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2646 src/hydrogen.cc:2646: static void RemoveCheck(HBoundsCheck* check) { I think this method should be generalized and moved to HTemplateInstruction (under a better name). In general, static methods are a code smell, with very few exceptions like factory methods. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2659 src/hydrogen.cc:2659: if (check_simulate_ != NULL) { Use a guard clause instead, see http://martinfowler.com/refactoring/catalog/replaceNestedConditionalWithGuard... http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2689 src/hydrogen.cc:2689: if (check->next()->IsSimulate()) { Using a ternary operator here and using initializer syntax instead is more "C++"-like. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2720 src/hydrogen.cc:2720: intptr_t hash = key->Hash(); Wrong type, but this is nicer when inlined, anyway. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2726 src/hydrogen.cc:2726: intptr_t hash = key->Hash(); Inline again. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2727 src/hydrogen.cc:2727: ZoneHashMap::Entry* entry = Lookup(key, hash, true); No need to name this explicitly. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2749 src/hydrogen.cc:2749: if (i->IsBoundsCheck()) { 'if (!i->IsBoundsCheck()) continue;' reduces nesting a bit and is clearer IMHO. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2769 src/hydrogen.cc:2769: BoundsCheckBbData::RemoveCheck(check); This will probably be something like 'check->Remove()' or whatever the function will be called. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2773 src/hydrogen.cc:2773: isolate()->counters()->array_bounds_checks_removed()->Increment(); I don't think that the increment here is correct, because the check is removed conditionally.
http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2639 src/hydrogen.cc:2639: BoundsCheckKey* Key() {return key_;} style-nit: add spaces after/before {}. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2671 src/hydrogen.cc:2671: added_index_->InsertBefore(added_simulate); I think this is a little problematic and can be simplified: You only need an HSimulate if the HAdd has side-effects (i.e. if it is a tagged-add). In this case it is not safe to move it around in any case, since it can have side effects.) If you limit yourself to index expressions a[i+c] where the index i+c is an integer add (hadd->representation()->IsInteger32()), then you don't need to worry about the HSimulate at all and you can insert it anywhere without creating a new HSimulate. http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2682 src/hydrogen.cc:2682: BoundsCheckBbData(BoundsCheckKey* key, int32_t offset, HBasicBlock* bb, style-nit: One parameter pre line in definitions and declarations.
http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2671 src/hydrogen.cc:2671: added_index_->InsertBefore(added_simulate); On 2012/04/13 12:38:00, fschneider wrote: > I think this is a little problematic and can be simplified: You only need an > HSimulate if the HAdd has side-effects (i.e. if it is a tagged-add). In this > case it is not safe to move it around in any case, since it can have side > effects.) > > If you limit yourself to index expressions a[i+c] where the index i+c is an > integer add (hadd->representation()->IsInteger32()), then you don't need to > worry about the HSimulate at all and you can insert it anywhere without creating > a new HSimulate. Good point, but I suggest using 'foo->HasObservableSideEffects()' instead of 'foo->representation()->IsInteger32()'. The side effects are the real reason for disallowing this, and it's more consistent with HInstruction::Verify().
On 2012/04/13 12:49:33, Sven Panne wrote: > http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc > File src/hydrogen.cc (right): > > http://codereview.chromium.org/10032029/diff/4001/src/hydrogen.cc#newcode2671 > src/hydrogen.cc:2671: added_index_->InsertBefore(added_simulate); > On 2012/04/13 12:38:00, fschneider wrote: > > I think this is a little problematic and can be simplified: You only need an > > HSimulate if the HAdd has side-effects (i.e. if it is a tagged-add). In this > > case it is not safe to move it around in any case, since it can have side > > effects.) > > > > If you limit yourself to index expressions a[i+c] where the index i+c is an > > integer add (hadd->representation()->IsInteger32()), then you don't need to > > worry about the HSimulate at all and you can insert it anywhere without > creating > > a new HSimulate. > > Good point, but I suggest using 'foo->HasObservableSideEffects()' instead of > 'foo->representation()->IsInteger32()'. The side effects are the real reason for > disallowing this, and it's more consistent with HInstruction::Verify(). Actually even simpler: I'd expect that you can ASSERT that, if the index expression is an HAdd, that the HAdd has int32 representation since the fast element load/store requires the index to be int32.
On 2012/04/13 13:16:59, fschneider wrote: > Actually even simpler: I'd expect that you can ASSERT that, if the index > expression is an HAdd, that the HAdd has int32 representation since the fast > element load/store requires the index to be int32. Even simpler: the index must have int32 representation anyway (even if it is not an add). And I do not need the HAdd anymore to get the base index: I have it in the Key object.
Everything should have been addressed at this point... https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc File src/hydrogen.cc (right): https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2592: if (check->index()->IsAdd()) { On 2012/04/13 11:38:06, Sven Panne wrote: > If we extract a function like 'MatchOffsetExpresssion(HValue** index_base, > HConstant** constant)' and massage CoverCheck a tiny bit, it should be > straightforward to handle the HMinus case, too. I leave it up to you to do this > now or in a separate CL. I'll do it in a separate CL. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2637: // class BoundsCheckBbData { On 2012/04/13 11:38:06, Sven Panne wrote: > Remove that line. Done. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2639: BoundsCheckKey* Key() {return key_;} On 2012/04/13 12:38:00, fschneider wrote: > style-nit: add spaces after/before {}. Done. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2646: static void RemoveCheck(HBoundsCheck* check) { On 2012/04/13 11:38:06, Sven Panne wrote: > I think this method should be generalized and moved to HTemplateInstruction > (under a better name). In general, static methods are a code smell, with very > few exceptions like factory methods. Done. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2659: if (check_simulate_ != NULL) { On 2012/04/13 11:38:06, Sven Panne wrote: > Use a guard clause instead, see > http://martinfowler.com/refactoring/catalog/replaceNestedConditionalWithGuard... Done. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2671: added_index_->InsertBefore(added_simulate); I ended up asserting that the index representation is int32 because it must be true and it's enough for what we need. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2682: BoundsCheckBbData(BoundsCheckKey* key, int32_t offset, HBasicBlock* bb, On 2012/04/13 12:38:00, fschneider wrote: > style-nit: One parameter pre line in definitions and declarations. Done. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2689: if (check->next()->IsSimulate()) { On 2012/04/13 11:38:06, Sven Panne wrote: > Using a ternary operator here and using initializer syntax instead is more > "C++"-like. Done. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2720: intptr_t hash = key->Hash(); On 2012/04/13 11:38:06, Sven Panne wrote: > Wrong type, but this is nicer when inlined, anyway. Done. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2726: intptr_t hash = key->Hash(); On 2012/04/13 11:38:06, Sven Panne wrote: > Inline again. Done. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2727: ZoneHashMap::Entry* entry = Lookup(key, hash, true); On 2012/04/13 11:38:06, Sven Panne wrote: > No need to name this explicitly. Done. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2749: if (i->IsBoundsCheck()) { On 2012/04/13 11:38:06, Sven Panne wrote: > 'if (!i->IsBoundsCheck()) continue;' reduces nesting a bit and is clearer IMHO. Done. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2769: BoundsCheckBbData::RemoveCheck(check); On 2012/04/13 11:38:06, Sven Panne wrote: > This will probably be something like 'check->Remove()' or whatever the function > will be called. Done. https://chromiumcodereview.appspot.com/10032029/diff/4001/src/hydrogen.cc#new... src/hydrogen.cc:2773: isolate()->counters()->array_bounds_checks_removed()->Increment(); On 2012/04/13 11:38:06, Sven Panne wrote: > I don't think that the increment here is correct, because the check is removed > conditionally. Done.
http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc#newcode2639 src/hydrogen.cc:2639: int32_t Offset() {return offset_;} Spaces around { and }. http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc#newcode2650 src/hydrogen.cc:2650: Representation::Integer32()); Indentation. http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc#newcode2694 src/hydrogen.cc:2694: Two empty lines here. http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc#newcode2701 src/hydrogen.cc:2701: Two lines also here (and in other places between functions and classes) http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc#newcode2705 src/hydrogen.cc:2705: return reinterpret_cast<BoundsCheckBbData**>(&(Lookup(key, I'd break the line before the &. I think it will be better readable. http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc#newcode2738 src/hydrogen.cc:2738: // TODO(mmassi): allocate key only when we create a new table entry... To solve this TODO you can maybe consider making BoundsCheckKey BASE_EMBEDDED (i.e. not zone-allocated). Then you don't need to heap-allocate the space for it except when creating a new BoundsCheckBbData object.
Fixed style issues and shortened the phase name to help Golem understand it. At this point it should be committable. http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc#newcode2639 src/hydrogen.cc:2639: int32_t Offset() {return offset_;} On 2012/04/18 07:51:35, fschneider wrote: > Spaces around { and }. Done. http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc#newcode2650 src/hydrogen.cc:2650: Representation::Integer32()); On 2012/04/18 07:51:35, fschneider wrote: > Indentation. Done. http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc#newcode2694 src/hydrogen.cc:2694: On 2012/04/18 07:51:35, fschneider wrote: > Two empty lines here. Done. http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc#newcode2701 src/hydrogen.cc:2701: On 2012/04/18 07:51:35, fschneider wrote: > Two lines also here (and in other places between functions and classes) Done. http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc#newcode2705 src/hydrogen.cc:2705: return reinterpret_cast<BoundsCheckBbData**>(&(Lookup(key, On 2012/04/18 07:51:35, fschneider wrote: > I'd break the line before the &. I think it will be better readable. Done. http://codereview.chromium.org/10032029/diff/14001/src/hydrogen.cc#newcode2738 src/hydrogen.cc:2738: // TODO(mmassi): allocate key only when we create a new table entry... On 2012/04/18 07:51:35, fschneider wrote: > To solve this TODO you can maybe consider making BoundsCheckKey BASE_EMBEDDED We decided to leave the code as it is and remove the TODO. This should cost us two pointers in the zone for every bounds check, which is not *that* much. The alternative is not hard to implement but the code would be less clean, and the maintainability cost is likely higher than the memory cost.
LGTM if you add a unit test. Please add a mjsunit test that covers all the cases of your analysis: Hoisting of checks with positive and negative offsets a[i] where i = expr-1, expr, expr+1, etc. within a block, and also elimination of covered checks across dominated blocks, etc. You can look at existing test files how to force optimization of a function (search for OptimizeFunctionOnNextCall). http://codereview.chromium.org/10032029/diff/16001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/16001/src/hydrogen.cc#newcode2621 src/hydrogen.cc:2621: Two lines space here. http://codereview.chromium.org/10032029/diff/16001/src/hydrogen.cc#newcode2694 src/hydrogen.cc:2694: Two lines space here. http://codereview.chromium.org/10032029/diff/16001/src/hydrogen.cc#newcode2701 src/hydrogen.cc:2701: Two lines space here. http://codereview.chromium.org/10032029/diff/16001/src/hydrogen.cc#newcode2720 src/hydrogen.cc:2720: Two lines space here. http://codereview.chromium.org/10032029/diff/16001/src/hydrogen.cc#newcode2737 src/hydrogen.cc:2737: // TODO(mmassi): allocate key only when we create a new table entry... If we plan to implement the TODO, we usually create an issue on the v8 issue tracker and write TODO(issue number). As discussed offline, we leave it as is for now, so I'd remove the TODO-line completely. http://codereview.chromium.org/10032029/diff/16001/src/hydrogen.cc#newcode2781 src/hydrogen.cc:2781: Two lines space here.
the last patch set should address everything... https://chromiumcodereview.appspot.com/10032029/diff/16001/src/hydrogen.cc File src/hydrogen.cc (right): https://chromiumcodereview.appspot.com/10032029/diff/16001/src/hydrogen.cc#ne... src/hydrogen.cc:2621: On 2012/04/18 09:42:18, fschneider wrote: > Two lines space here. Done. https://chromiumcodereview.appspot.com/10032029/diff/16001/src/hydrogen.cc#ne... src/hydrogen.cc:2694: On 2012/04/18 09:42:18, fschneider wrote: > Two lines space here. Done. https://chromiumcodereview.appspot.com/10032029/diff/16001/src/hydrogen.cc#ne... src/hydrogen.cc:2701: On 2012/04/18 09:42:18, fschneider wrote: > Two lines space here. Done. https://chromiumcodereview.appspot.com/10032029/diff/16001/src/hydrogen.cc#ne... src/hydrogen.cc:2720: On 2012/04/18 09:42:18, fschneider wrote: > Two lines space here. Done. https://chromiumcodereview.appspot.com/10032029/diff/16001/src/hydrogen.cc#ne... src/hydrogen.cc:2781: On 2012/04/18 09:42:18, fschneider wrote: > Two lines space here. Done.
Patch 7 fixes all known issues, and passes a "golem sanity check". It does not cause regressions and produces some stable speedups (bulletben +1.1, others something similar).
Almost there. Mostly style comments left: http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2617 src/hydrogen.cc:2617: *offset = is_sub ? - constant->Integer32Value() I'd break this expression like this: *offset = is_sub ? -constant->Integer32Value() : constant->Integer32Value(); http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2628 src/hydrogen.cc:2628: BoundsCheckKey(HValue* index_base, HValue* length) { This is may be rewritten shorter using an initializer list: BoundsCheckKey(HValue* index_base, HValue* length) : index_base_(index_base), length_(length) { } http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2763 src/hydrogen.cc:2763: original_value, Don't the arguments fit on one line? http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2775 src/hydrogen.cc:2775: HConstant** constant) { Fits on the previous line? http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2778 src/hydrogen.cc:2778: (*constant)->Unlink(); We use normally the helper DeleteAndReplaceWith(NULL) for deleting instructions from the graph. It has some additional assertions that the value does not have any uses left. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2807 src/hydrogen.cc:2807: } Fits on the previous line. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2835 src/hydrogen.cc:2835: bb_data_list = new(bb->zone()) BoundsCheckBbData(key, Since HGraph already has a zone() accessor, you can just write: new (zone()) http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2850 src/hydrogen.cc:2850: int32_t new_lower_offset = offset < data->LowerOffset() ? offset Maybe the expressions here better readable as: int32_t new_lower_offset = offset < data->LowerOffset() ? offset : data->LowerOffset(); http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2884 src/hydrogen.cc:2884: AssertNoAllocation no_gc; Why would you want to assert no allocation here? It does not hurt, but does not seem necessary. We use this assert in places where we use raw pointers to the JS heap instead of handles (e.g. many function in objects.cc, runtime.cc).
Patch 8 addresses the last round of comments. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2617 src/hydrogen.cc:2617: *offset = is_sub ? - constant->Integer32Value() On 2012/04/25 09:40:48, fschneider wrote: > I'd break this expression like this: > > *offset = is_sub ? -constant->Integer32Value() > : constant->Integer32Value(); Done. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2628 src/hydrogen.cc:2628: BoundsCheckKey(HValue* index_base, HValue* length) { > BoundsCheckKey(HValue* index_base, HValue* length) > : index_base_(index_base), > length_(length) { } Done but with the colon be on the 1st line: is it ok anyway? http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2763 src/hydrogen.cc:2763: original_value, On 2012/04/25 09:40:48, fschneider wrote: > Don't the arguments fit on one line? No: 81 chars wide :-( http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2775 src/hydrogen.cc:2775: HConstant** constant) { On 2012/04/25 09:40:48, fschneider wrote: > Fits on the previous line? Done. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2778 src/hydrogen.cc:2778: (*constant)->Unlink(); On 2012/04/25 09:40:48, fschneider wrote: > We use normally the helper DeleteAndReplaceWith(NULL) for deleting instructions > from the graph. It has some additional assertions that the value does not have > any uses left. Done. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2807 src/hydrogen.cc:2807: } On 2012/04/25 09:40:48, fschneider wrote: > Fits on the previous line. Done. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2835 src/hydrogen.cc:2835: bb_data_list = new(bb->zone()) BoundsCheckBbData(key, On 2012/04/25 09:40:48, fschneider wrote: > Since HGraph already has a zone() accessor, you can just write: > > new (zone()) Done. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2850 src/hydrogen.cc:2850: int32_t new_lower_offset = offset < data->LowerOffset() ? offset On 2012/04/25 09:40:48, fschneider wrote: > Maybe the expressions here better readable as: > > int32_t new_lower_offset = offset < data->LowerOffset() > ? offset > : data->LowerOffset(); Done. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2884 src/hydrogen.cc:2884: AssertNoAllocation no_gc; On 2012/04/25 09:40:48, fschneider wrote: > Why would you want to assert no allocation here? It does not hurt, but does not > seem necessary. It is necessary: I was hitting asserts without it. This happened computing key hashes calling HConstant::Hashcode().
Fixed colon position in initializers. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2628 src/hydrogen.cc:2628: BoundsCheckKey(HValue* index_base, HValue* length) { On 2012/04/25 12:19:19, Massi wrote: > Done but with the colon be on the 1st line: is it ok anyway? Fixed this and another one...
Looks almost good! Can you also fix the redundant index-additions that may arrive in the example we discussed offline? I think it will improve the benefits of your optimization noticable. http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/28001/src/hydrogen.cc#newcode2884 src/hydrogen.cc:2884: AssertNoAllocation no_gc; On 2012/04/25 12:19:19, Massi wrote: > On 2012/04/25 09:40:48, fschneider wrote: > > Why would you want to assert no allocation here? It does not hurt, but does > not > > seem necessary. > > It is necessary: I was hitting asserts without it. > This happened computing key hashes calling HConstant::Hashcode(). I don't believe it is necessary. If anything it will trigger an assert once you allocate a heap object on the JS heap in your code. I don't this is currently the case, and even if it would, we can deal with allocations inside the compiler since everything is handlified. http://codereview.chromium.org/10032029/diff/33002/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/10032029/diff/33002/src/hydrogen.cc#newcode2687 src/hydrogen.cc:2687: int32_t new_offset) { Fits in previous line? http://codereview.chromium.org/10032029/diff/33002/src/hydrogen.cc#newcode2775 src/hydrogen.cc:2775: (*add)->DeleteAndReplaceWith((*add)->left()); Could you add a test case that exercises this code inside the if-statement? When running your added it, I don't find this path to be executed. http://codereview.chromium.org/10032029/diff/33002/test/mjsunit/array-bounds-... File test/mjsunit/array-bounds-check-removal.js (right): http://codereview.chromium.org/10032029/diff/33002/test/mjsunit/array-bounds-... test/mjsunit/array-bounds-check-removal.js:144: gc(); Why do you need --expose-gc and a call gc() here? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
