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