Improve dead code elimination by transitively marking live code and removing all dead code. Replace…

Improve dead code elimination by transitively marking live code and removing all dead code. Replace unreachable phi removal algorithm with the new dead code elimination pass, which is more thorough.

Committed: https://code.google.com/p/v8/source/detail?r=14661

[Sven Panne, 2013-05-07 13:32:10]

First round of comments, adding Jakob for a 2nd opinion on the 1st DCE
phase/flag.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1900: stream->Add("%s%s]",
Do we never print phis marked as dead by DCE? If this is the case, removing this
piece information is OK here (at least if you adapt the format pattern ;-). If
not, print it.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:814: kIsNotDead,
I don't like negated things, especially when the uses are negated, too.
Something like kAlive or kReachable (choose some positive formulation) is
better.

http://www.refactoring.com/catalog/removeDoubleNegative.html

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1083: return HasObservableSideEffects() ||
!IsDeletable();
Hmmm, do we really need IsDead *and* IsNotDeletable? !IsNotDeletable is a part
of IsDead, BTW. Again, negated things are not nice, see above.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14676011/diff/1/src/hydrogen.cc#newcode4858
src/hydrogen.cc:4858: if (FLAG_dead_code_elimination) DeadCodeElimination();
I am not sure if this is what we want: Almost all our bots run without DCE,
because our tests consist of a lot of dead code. If we don't do this phase here,
we fundamentally change things. Perhaps keep the old flag for this DCE phase?
I'm a bit torn, adding Jakob for a 2nd opinion...

https://codereview.chromium.org/14676011/diff/1/src/hydrogen.cc#newcode5390
src/hydrogen.cc:5390: ALLOW_HANDLE_DEREF(isolate(), "trace mode for deadcode");
Why exactly do we need this and why is this safe?

[Jakob Kummerow, 2013-05-08 12:07:24]

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:813: // used during dead code elimination to
indicate the instruction is live
nit: Our style guide demands that comments be proper sentences, beginning with a
capital letter and ending with appropriate punctuation.
This remark applies to most comments introduced in this CL.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1077: bool IsDead() const {
I think this can be removed. It only had two call sites AFAICS, and both are
deleted in this CL.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14676011/diff/1/src/hydrogen.cc#newcode4858
src/hydrogen.cc:4858: if (FLAG_dead_code_elimination) DeadCodeElimination();
On 2013/05/07 13:32:10, Sven Panne wrote:
> I am not sure if this is what we want: Almost all our bots run without DCE,
> because our tests consist of a lot of dead code. If we don't do this phase
here,
> we fundamentally change things. Perhaps keep the old flag for this DCE phase?
> I'm a bit torn, adding Jakob for a 2nd opinion...

As discussed offline, it's probably OK to leave this as it is. We'll have more
dead phis when running tests, but that shouldn't affect coverage negatively.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen.cc#newcode5420
src/hydrogen.cc:5420: ZoneList<HPhi*> philist(blocks_.length(), zone());
Why do we need this phi list at all? Can't we simply iterate through all blocks
and each block's phi list when trying to eliminate dead phis? (I.e. have a loop
similar to the one in lines 5431-5435 inserted before 5454, instead of the loop
in 5457-5463). 
Remember, zone memory can't be freed unless the entire zone is nuked. Zone
allocation is fast, but the memory used by the objects stays around when the C++
variables go out of scope.

[Sven Panne, 2013-05-13 09:31:55]

Regarding the unit tests: Unless there is a deep reason (which I currently can't
see), I would prefer having all tests in a single file. We usually group related
tests together like this.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1901: IsConvertibleToInteger() ? "" : "_ncti");
Nit: This probably fits into the previous line.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5392
src/hydrogen.cc:5392: if (ref != NULL) ref->PrintNameTo(&stream);
Please print the full instruction here, not just the variable name, otherwise
the output is not very helpful. Same for instr below. (We did this previously
BTW).

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5401
src/hydrogen.cc:5401: bool HGraph::TryDeadCodeElimination(HValue* instr) {
I think this helper method is actually less clear than inlining it at its call
sites and simplifying the resulting code.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5417
src/hydrogen.cc:5417: HPhase phase("H_Dead code elimination", this);
We call this phase twice, so it would be good to name these differently, making
the visualizer output more readable. Note that HPhase doesn't keep a copy of the
string, just a pointer.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5419
src/hydrogen.cc:5419: ZoneList<HPhi*> philist(blocks_.length(), zone());
As Jakob already pointed out, I don't think we need this, see below.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5421
src/hydrogen.cc:5421: // Mark initial root instructions for dead code
elimination.
Replace this comment and the ones below by extracted methods with explanatory
names (just proposals below):

   MarkRoots(&worklist);
   MarkLive(&worklist);
   Sweep();

http://www.refactoring.com/catalog/extractMethod.html ;-)

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5456
src/hydrogen.cc:5456: while (!philist.is_empty()) {
Just fold this into the previous loop, no need for a separate list.

[titzer, 2013-05-13 13:29:30]

On 2013/05/13 09:31:55, Sven Panne wrote:
> Regarding the unit tests: Unless there is a deep reason (which I currently
can't
> see), I would prefer having all tests in a single file. We usually group
related
> tests together like this.

Smaller tests mean better parallelization and an easier time debugging :)

> 
>
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen-instructions.cc
> File src/hydrogen-instructions.cc (right):
> 
>
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen-instructions...
> src/hydrogen-instructions.cc:1901: IsConvertibleToInteger() ? "" : "_ncti");
> Nit: This probably fits into the previous line.
> 
> https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc
> File src/hydrogen.cc (right):
> 
>
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5392
> src/hydrogen.cc:5392: if (ref != NULL) ref->PrintNameTo(&stream);
> Please print the full instruction here, not just the variable name, otherwise
> the output is not very helpful. Same for instr below. (We did this previously
> BTW).
> 
>
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5401
> src/hydrogen.cc:5401: bool HGraph::TryDeadCodeElimination(HValue* instr) {
> I think this helper method is actually less clear than inlining it at its call
> sites and simplifying the resulting code.
> 
>
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5417
> src/hydrogen.cc:5417: HPhase phase("H_Dead code elimination", this);
> We call this phase twice, so it would be good to name these differently,
making
> the visualizer output more readable. Note that HPhase doesn't keep a copy of
the
> string, just a pointer.
> 
>
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5419
> src/hydrogen.cc:5419: ZoneList<HPhi*> philist(blocks_.length(), zone());
> As Jakob already pointed out, I don't think we need this, see below.
> 
>
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5421
> src/hydrogen.cc:5421: // Mark initial root instructions for dead code
> elimination.
> Replace this comment and the ones below by extracted methods with explanatory
> names (just proposals below):
> 
>    MarkRoots(&worklist);
>    MarkLive(&worklist);
>    Sweep();
> 
> http://www.refactoring.com/catalog/extractMethod.html ;-)
> 
>
https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5456
> src/hydrogen.cc:5456: while (!philist.is_empty()) {
> Just fold this into the previous loop, no need for a separate list.

[titzer, 2013-05-13 13:29:38]

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:1900: stream->Add("%s%s]",
On 2013/05/07 13:32:10, Sven Panne wrote:
> Do we never print phis marked as dead by DCE? If this is the case, removing
this
> piece information is OK here (at least if you adapt the format pattern ;-). If
> not, print it.

Done.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:813: // used during dead code elimination to
indicate the instruction is live
On 2013/05/08 12:07:24, Jakob wrote:
> nit: Our style guide demands that comments be proper sentences, beginning with
a
> capital letter and ending with appropriate punctuation.
> This remark applies to most comments introduced in this CL.

Done.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:814: kIsNotDead,
On 2013/05/07 13:32:10, Sven Panne wrote:
> I don't like negated things, especially when the uses are negated, too.
> Something like kAlive or kReachable (choose some positive formulation) is
> better.
> 
> http://www.refactoring.com/catalog/removeDoubleNegative.html

Done.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1077: bool IsDead() const {
On 2013/05/08 12:07:24, Jakob wrote:
> I think this can be removed. It only had two call sites AFAICS, and both are
> deleted in this CL.

Done.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1083: return HasObservableSideEffects() ||
!IsDeletable();
On 2013/05/07 13:32:10, Sven Panne wrote:
> Hmmm, do we really need IsDead *and* IsNotDeletable? !IsNotDeletable is a part
> of IsDead, BTW. Again, negated things are not nice, see above.

Done.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14676011/diff/1/src/hydrogen.cc#newcode4858
src/hydrogen.cc:4858: if (FLAG_dead_code_elimination) DeadCodeElimination();
On 2013/05/08 12:07:24, Jakob wrote:
> On 2013/05/07 13:32:10, Sven Panne wrote:
> > I am not sure if this is what we want: Almost all our bots run without DCE,
> > because our tests consist of a lot of dead code. If we don't do this phase
> here,
> > we fundamentally change things. Perhaps keep the old flag for this DCE
phase?
> > I'm a bit torn, adding Jakob for a 2nd opinion...
> 
> As discussed offline, it's probably OK to leave this as it is. We'll have more
> dead phis when running tests, but that shouldn't affect coverage negatively.

Leaving as one flag.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen.cc#newcode5390
src/hydrogen.cc:5390: ALLOW_HANDLE_DEREF(isolate(), "trace mode for deadcode");
On 2013/05/07 13:32:10, Sven Panne wrote:
> Why exactly do we need this and why is this safe?

I was debugging and printing the entire instruction, but I guess it is not
necessary to print only the name. Removed.

https://codereview.chromium.org/14676011/diff/1/src/hydrogen.cc#newcode5420
src/hydrogen.cc:5420: ZoneList<HPhi*> philist(blocks_.length(), zone());
On 2013/05/08 12:07:24, Jakob wrote:
> Why do we need this phi list at all? Can't we simply iterate through all
blocks
> and each block's phi list when trying to eliminate dead phis? (I.e. have a
loop
> similar to the one in lines 5431-5435 inserted before 5454, instead of the
loop
> in 5457-5463). 
> Remember, zone memory can't be freed unless the entire zone is nuked. Zone
> allocation is fast, but the memory used by the objects stays around when the
C++
> variables go out of scope.

Because if the code iterates over the phi list as it deletes phis, the list
changes and it misses some.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1901: IsConvertibleToInteger() ? "" : "_ncti");
On 2013/05/13 09:31:55, Sven Panne wrote:
> Nit: This probably fits into the previous line.

Done.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5392
src/hydrogen.cc:5392: if (ref != NULL) ref->PrintNameTo(&stream);
On 2013/05/13 09:31:55, Sven Panne wrote:
> Please print the full instruction here, not just the variable name, otherwise
> the output is not very helpful. Same for instr below. (We did this previously
> BTW).

Done.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5401
src/hydrogen.cc:5401: bool HGraph::TryDeadCodeElimination(HValue* instr) {
On 2013/05/13 09:31:55, Sven Panne wrote:
> I think this helper method is actually less clear than inlining it at its call
> sites and simplifying the resulting code.

I would go for inlining if it weren't used in two places.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5417
src/hydrogen.cc:5417: HPhase phase("H_Dead code elimination", this);
On 2013/05/13 09:31:55, Sven Panne wrote:
> We call this phase twice, so it would be good to name these differently,
making
> the visualizer output more readable. Note that HPhase doesn't keep a copy of
the
> string, just a pointer.

Done.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5419
src/hydrogen.cc:5419: ZoneList<HPhi*> philist(blocks_.length(), zone());
On 2013/05/13 09:31:55, Sven Panne wrote:
> As Jakob already pointed out, I don't think we need this, see below.

I forgot to hit "send" on a previous set of comments. Maybe it will come this
time. But at any rate, we process the phis separately to avoid mutating the phi
list for a block as we iterate over it.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5421
src/hydrogen.cc:5421: // Mark initial root instructions for dead code
elimination.
On 2013/05/13 09:31:55, Sven Panne wrote:
> Replace this comment and the ones below by extracted methods with explanatory
> names (just proposals below):
> 
>    MarkRoots(&worklist);
>    MarkLive(&worklist);
>    Sweep();
> 
> http://www.refactoring.com/catalog/extractMethod.html ;-)

Normally I would agree with breaking things into smaller methods (especially if
those methods can then be reused), but my opinion in this case is that the
overhead of adding the declarations here and in the header file aren't worth the
savings.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5456
src/hydrogen.cc:5456: while (!philist.is_empty()) {
On 2013/05/13 09:31:55, Sven Panne wrote:
> Just fold this into the previous loop, no need for a separate list.

See above; we can't because the phi list of a block changes as we remove items
from it.

[Sven Panne, 2013-05-13 14:55:52]

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1901: IsConvertibleToInteger() ? "" : "_ncti");
On 2013/05/13 13:29:39, titzer wrote:
> On 2013/05/13 09:31:55, Sven Panne wrote:
> > Nit: This probably fits into the previous line.
> 
> Done.

Not really... ;-)

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5392
src/hydrogen.cc:5392: if (ref != NULL) ref->PrintNameTo(&stream);
On 2013/05/13 13:29:39, titzer wrote:
> On 2013/05/13 09:31:55, Sven Panne wrote:
> > Please print the full instruction here, not just the variable name,
otherwise
> > the output is not very helpful. Same for instr below. (We did this
previously
> > BTW).
> 
> Done.

Again, I don't see the "Done" part, perhaps you forgot to upload a new version?

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5401
src/hydrogen.cc:5401: bool HGraph::TryDeadCodeElimination(HValue* instr) {
If you insist of keeping it, then make it at least remove the boolean return
value, it's a bit confusing. Instead just check if you've removed a phi and if
yes, call RecordDeletedPhi on the phi's block. Renaming this helper function to
"RemoveIfDead" (or something like this) would be nice, too.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5417
src/hydrogen.cc:5417: HPhase phase("H_Dead code elimination", this);
On 2013/05/13 13:29:39, titzer wrote:
> On 2013/05/13 09:31:55, Sven Panne wrote:
> > We call this phase twice, so it would be good to name these differently,
> making
> > the visualizer output more readable. Note that HPhase doesn't keep a copy of
> the
> > string, just a pointer.
> 
> Done.

Where? :-)

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5419
src/hydrogen.cc:5419: ZoneList<HPhi*> philist(blocks_.length(), zone());
Then rename it to phi_snapshot, making it's purpose clear.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5421
src/hydrogen.cc:5421: // Mark initial root instructions for dead code
elimination.
In my opinion this is a *very* worthwhile refactoring: If you feel the slightest
urge to comment something (as you obviously do), don't comment it, but refactor
it instead. In the words of the famous Uncle Bob: Comments are a sign of our
inability to express our thoughts in code. :-)

One could even go as far as insisting on making every compiler phase a separate
class, which we should probably do in the future. In that case, the functions I
proposed would be the natural private helper functions.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5456
src/hydrogen.cc:5456: while (!philist.is_empty()) {
An alternative approach would be folding this into the previous loop and
collecting the phis to nuke in a (probably very small) list, and iterate over
that list later to actually kill them. But I leave that up to you...

[titzer, 2013-05-13 18:11:13]

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen-instructions...
src/hydrogen-instructions.cc:1901: IsConvertibleToInteger() ? "" : "_ncti");
On 2013/05/13 14:55:52, Sven Panne wrote:
> On 2013/05/13 13:29:39, titzer wrote:
> > On 2013/05/13 09:31:55, Sven Panne wrote:
> > > Nit: This probably fits into the previous line.
> > 
> > Done.
> 
> Not really... ;-)

I think I got it this time. :)

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5392
src/hydrogen.cc:5392: if (ref != NULL) ref->PrintNameTo(&stream);
On 2013/05/13 14:55:52, Sven Panne wrote:
> On 2013/05/13 13:29:39, titzer wrote:
> > On 2013/05/13 09:31:55, Sven Panne wrote:
> > > Please print the full instruction here, not just the variable name,
> otherwise
> > > the output is not very helpful. Same for instr below. (We did this
> previously
> > > BTW).
> > 
> > Done.
> 
> Again, I don't see the "Done" part, perhaps you forgot to upload a new
version?

...

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5401
src/hydrogen.cc:5401: bool HGraph::TryDeadCodeElimination(HValue* instr) {
On 2013/05/13 14:55:52, Sven Panne wrote:
> If you insist of keeping it, then make it at least remove the boolean return
> value, it's a bit confusing. Instead just check if you've removed a phi and if
> yes, call RecordDeletedPhi on the phi's block. Renaming this helper function
to
> "RemoveIfDead" (or something like this) would be nice, too.

I've inlined it and split the main algorithm into two methods.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5417
src/hydrogen.cc:5417: HPhase phase("H_Dead code elimination", this);
On 2013/05/13 14:55:52, Sven Panne wrote:
> On 2013/05/13 13:29:39, titzer wrote:
> > On 2013/05/13 09:31:55, Sven Panne wrote:
> > > We call this phase twice, so it would be good to name these differently,
> > making
> > > the visualizer output more readable. Note that HPhase doesn't keep a copy
of
> > the
> > > string, just a pointer.
> > 
> > Done.
> 
> Where? :-)

Done.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5419
src/hydrogen.cc:5419: ZoneList<HPhi*> philist(blocks_.length(), zone());
On 2013/05/13 14:55:52, Sven Panne wrote:
> Then rename it to phi_snapshot, making it's purpose clear.

I'm going with your latter suggestion to make a separate list of dead phis.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5421
src/hydrogen.cc:5421: // Mark initial root instructions for dead code
elimination.
On 2013/05/13 14:55:52, Sven Panne wrote:
> In my opinion this is a *very* worthwhile refactoring: If you feel the
slightest
> urge to comment something (as you obviously do), don't comment it, but
refactor
> it instead. In the words of the famous Uncle Bob: Comments are a sign of our
> inability to express our thoughts in code. :-)
> 

I'm gonna go ahead and disagree with Bob on this one and leave my comments in.
In the future, I will endeavor to have fewer thoughts :)

> One could even go as far as insisting on making every compiler phase a
separate
> class, which we should probably do in the future. In that case, the functions
I
> proposed would be the natural private helper functions.

Another day, perhaps.

https://codereview.chromium.org/14676011/diff/10007/src/hydrogen.cc#newcode5456
src/hydrogen.cc:5456: while (!philist.is_empty()) {
On 2013/05/13 14:55:52, Sven Panne wrote:
> An alternative approach would be folding this into the previous loop and
> collecting the phis to nuke in a (probably very small) list, and iterate over
> that list later to actually kill them. But I leave that up to you...

Yeah, that's a better idea.

[Sven Panne, 2013-05-14 07:57:31]

Final round, I think. :-) Apart from the IsDeletable comment, why do we have
several unit test files? (This was in my previous comments, too.) I can neither
see an advantage in splitting the tests up nor some kind of reasoning behind the
current partitioning, which looks rather random to me.

https://codereview.chromium.org/14676011/diff/18001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/14676011/diff/18001/src/hydrogen.cc#newcode5425
src/hydrogen.cc:5425: if (phi->IsReceiver()) MarkLive(NULL, phi, &worklist);
Can we move this check into HPhi::IsDeletable? This way, we could consistently
write

  if (!instr->CanBeEliminated()) MarkLive(NULL, instr, &worklist);

[titzer, 2013-05-14 09:52:13]

On 2013/05/14 07:57:31, Sven Panne wrote:
> Final round, I think. :-) Apart from the IsDeletable comment, why do we have
> several unit test files? (This was in my previous comments, too.) I can
neither
> see an advantage in splitting the tests up nor some kind of reasoning behind
the
> current partitioning, which looks rather random to me.
> 

I did answer this before. In my experience there are four major advantages to
smaller tests:

1. better parallelization of tests
2. less code involved in debugging a failure, therefore easier  
3. testing the same feature with small variations can help pin down the source
of a bug, since it may fail in one situation but not another
4. It's easy to put small tests together into one big test. Vice versa is
harder.

> https://codereview.chromium.org/14676011/diff/18001/src/hydrogen.cc
> File src/hydrogen.cc (right):
> 
>
https://codereview.chromium.org/14676011/diff/18001/src/hydrogen.cc#newcode5425
> src/hydrogen.cc:5425: if (phi->IsReceiver()) MarkLive(NULL, phi, &worklist);
> Can we move this check into HPhi::IsDeletable? This way, we could consistently
> write
> 
>   if (!instr->CanBeEliminated()) MarkLive(NULL, instr, &worklist);

Done.

[Sven Panne, 2013-05-14 11:12:03]

LGTM. Note that my previous comments were about the random partitioning of DCE
tests into various files, not about the sizes of the individual tests (which are
small, which is good thing).

[titzer, 2013-05-14 13:11:04]

Committed patchset #6 manually as r14661 (presubmit successful).