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 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.
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.
