Streamline JumpTarget::ComputeEntryFrame by removing the separate loop...

Streamline JumpTarget::ComputeEntryFrame by removing the separate loop
that sets the static type on elements in the entry frame.  Instead set
the static type as early as possible when computing elements.

Because the elements are shared with the reaching frames, this has the
side effect of sometimes changing the static type of reaching frame
elements.  They will be changed anyway in VirtualFrame::PrepareMergeTo
immediately after, so this is safe.


Committed: http://code.google.com/p/v8/source/detail?r=1928

[Kevin Millikin (Chromium), 2009-05-12 11:41:44]

This represents a small improvement in compilation time.

[Lasse Reichstein, 2009-05-12 14:45:38]

LGTM

http://codereview.chromium.org/113258/diff/1/2
File src/jump-target.cc (right):

http://codereview.chromium.org/113258/diff/1/2#newcode118
Line 118: if (left->is_register() && right->is_register() &&
This function looks like a lot of duplicated code. The for conditions are
separate, even though they do almost, or exactly, the same. It's not easy to see
that they are all merely checking whether the two FrameElements are compatible,
and if they are, pick the unsynced one, if any.
When the bodies were simpler, it might not have been a problem, but I do think
it is now.

It seems like it could be simplified (of a sort) by combining the tree
conditions that all have the same body.

Something like: 
 if (... || ... || ...) {
   FrameElement result = left->is_synced ? right : left;
   result->set_static_type(static_type);
   return result;
 }
Perhaps make some utility function for the test, or for parts of the it. The one
at this comment looks like it could be named IsSameRegister.

You can even add the first test as well, since it doesn't seem to matter which
element is used.

http://codereview.chromium.org/113258/diff/1/2#newcode182
Line 182: FrameElement element = initial_frame->elements_[i];
If we look this element up again later, would it perhaps be easier to store a
pointer instead of making a copy?

http://codereview.chromium.org/113258/diff/1/2#newcode208
Line 208: elements[i] = Combine(element, &reaching_frames_[j]->elements_[i]);
Why do we read the element on each round of the inner loop, instead of reading
it before entering the loop, updating only the variable during the loop, and
then setting it back after the loop?

[Kevin Millikin (Chromium), 2009-05-13 07:37:05]

Thanks for the comments.  I'll address them in another change.