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#newcode1900
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#newcode813
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#newcode814
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#newcode1077
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#newcode1083
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.cc#newcode1901
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.
https://codereview.chromium.org/14676011/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
