Addressed comments. Starting with architecture ports. PTAL.
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen-escape-analysis.cc
File src/hydrogen-escape-analysis.cc (right):
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen-escape-analysis.cc#newcode74
src/hydrogen-escape-analysis.cc:74: for (int i = 0; i <
graph()->blocks()->length(); i++) {
On 2013/08/01 17:11:50, titzer wrote:
You can start i at allocate_block->id(), since the allocation cannot
dominate
blocks that come before.
Done. So obvious, I feel embarrassed now.
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen-escape-analysis.cc#newcode99
src/hydrogen-escape-analysis.cc:99: for (int index = 0; index <
number_of_values_; index++) {
On 2013/08/01 17:11:50, titzer wrote:
You're creating a lot of phis here. You only need to create a phi if
this block
is a loop header, or if the predecessors do not have the same HValue
for a
tracked field.
Done. Switched to lazy creation of phis only once one of the incoming
values is actually different from the others.
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen-escape-analysis.cc#newcode117
src/hydrogen-escape-analysis.cc:117: int index = load->access().offset()
/ kPointerSize;
On 2013/08/01 17:11:50, titzer wrote:
Are you only tracking in-object properties? Otherwise, you need to
check the
in-objectness of the access as well.
Done. Good point, currently we don't HAllocate objects that have
out-of-object fields and hence this doesn't happen. But I added asserts
to guard that.
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen-escape-analysis.cc#newcode189
src/hydrogen-escape-analysis.cc:189: SetStateAt(block, state);
On 2013/08/01 17:11:50, titzer wrote:
You should forward-propagate the block state to all successors of this
block;
then you can delete (or even reuse) the block state.
Done. Switched to forward propagation of the block state.
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen-escape-analysis.cc#newcode206
src/hydrogen-escape-analysis.cc:206: for (int index = 0; index <
number_of_values_; index++) {
On 2013/08/01 17:11:50, titzer wrote:
This is somewhat brittle because the phi input indexes must match up
with the
predecessor indexes, and since your edges are coming off a stack, I'm
really
surprised you were so lucky. This needs to be more robust somehow.
Done. Switched to use HBasicBlock::PredecessorIndexOf to always set
operands at the correct index. Also the post-processing step is obsolete
after switching to forward propagation.
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen-escape-analysis.h
File src/hydrogen-escape-analysis.h (right):
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen-escape-analysis.h#newcode61
src/hydrogen-escape-analysis.h:61: HCapturedObject*
NewState(HInstruction* previous) {
On 2013/08/01 17:11:50, titzer wrote:
Not sure why these New* methods are in the header. They seem pretty
complicated,
too.
Done. Moved their implementation into the .cc file.
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen-escape-analysis.h#newcode146
src/hydrogen-escape-analysis.h:146: BitVector loops_affected_by_stores_;
On 2013/08/01 17:11:50, titzer wrote:
As I understand it, you were going to track which loops had stores in
a later
CL. If so, you can just leave this guy out for now.
Done. Ooops, that is a leftover from a previous implementation and I
forgot to remove it. Removed.
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen-instructions.h#newcode3451
src/hydrogen-instructions.h:3451: ZoneList<HValue*> values_;
On 2013/08/01 17:11:50, titzer wrote:
How is this values_ array indexed? Does it store only in-object
properties and
is indexed by field index?
Correct. I added a comment to both HArgumentsObject and HCapturedObject
below that covers this.
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/21055011/diff/3001/src/hydrogen.cc#newcode2952
src/hydrogen.cc:2952:
On 2013/08/01 17:11:50, titzer wrote:
whitespace change
Done. Removed. Even though I think there should be whitespace here.
https://codereview.chromium.org/21055011/diff/3001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/21055011/diff/3001/src/ia32/lithium-codegen-ia32.cc#newcode785
src/ia32/lithium-codegen-ia32.cc:785: if
(environment->ObjectIsDuplicateAt(object_index)) {
On 2013/08/01 17:11:50, titzer wrote:
ObjectDuplicate means...object reference?
Correct. It means that a previously translated object is referenced
again by another slot (either on the stack or inside another
dematerialized object). I can rename all uses of "duplicated object" to
"referenced object" if you think that clarifies the situation. Let me
know.
https://codereview.chromium.org/21055011/diff/3001/src/lithium.h
File src/lithium.h (right):
https://codereview.chromium.org/21055011/diff/3001/src/lithium.h#newcode624
src/lithium.h:624: static LOperand* materialization_marker() { return
NULL; }
On 2013/08/01 17:11:50, titzer wrote:
NULL seems like it could get mixed up sometime. Maybe -1?
That would require many changes in the register allocator, because NULL
is the only LOperand being skipped there. We can do that, but I would
very much like to do that in another CL, not as part of this one.
https://codereview.chromium.org/21055011/
--
--
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.
