First round, want to finish this round early as it turns out I am writing
the
same comments a Ben. :)
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-gvn.cc
File src/hydrogen-gvn.cc (right):
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-gvn.cc#newcode779
src/hydrogen-gvn.cc:779: for (HInstructionIterator it(block);
!it.Done(); it.Advance()) {
Switching to the HInstructionIterator will break for cases where the
instruction is unlinked from the graph. See the following comment about
that.
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-gvn.cc#newcode799
src/hydrogen-gvn.cc:799: if (instr->IsLinked() && !flags.IsEmpty()) {
Instead of doing the IsLinked() checks here, could we just to a
"continue" after the side-effect dominator notification? Of course that
require us to make HInstructionIterator resilient against concurrent
modification (i.e. remember the "next_" field in the iterator).
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-gvn.cc#newcode807
src/hydrogen-gvn.cc:807: if (instr->IsLinked() &&
instr->CheckFlag(HValue::kUseGVN)) {
Likewise.
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.cc#newcode1658
src/hydrogen-instructions.cc:1658: HValue* dominator) {
nit: Indentation if off.
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.cc#newcode3206
src/hydrogen-instructions.cc:3206: HValue* dominator) {
nit: Indentation is off.
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.cc#newcode3208
src/hydrogen-instructions.cc:3208: if (FLAG_use_allocation_folding) {
Can we do early returns here instead of indenting the whole block, that
makes this whole method easier to read.
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.cc#newcode3240
src/hydrogen-instructions.cc:3240: HConstant* filler_map =
block->graph()->GetConstantFreeSpaceMap();
On 2013/07/08 12:40:14, titzer wrote:
Don't make this a global constant in the graph. That increases its
live range,
thus register pressure and spilling. It's fine to create this constant
at each
allocation site, IMO. Maybe if you have multiple folded allocations
you can
insert this constant after the intial HAllocate and reuse the same
one, but
making it global is probably not worth it.
I don't think this is an issue as the HConstant will not get a live
range. It will be embedded as a constant at every use-site.
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/18596005/diff/2001/src/hydrogen-instructions.h#newcode5811
src/hydrogen-instructions.h:5811: if (no_observable_side_effects) {
Can we add a TODO here that this argument should go away once we no
longer need to do initialization of folded allocation segments?
https://codereview.chromium.org/18596005/
--
--
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.
