[v8] [EMAIL PROTECTED] commented on revision r788.
Details are at http://code.google.com/p/v8/source/detail?r=788
Score: Neutral
General Comment:
More comments after submission.
-Ivan
PS. This is also an experiment to see how the after submission review tool
works.
Line-by-line comments:
File: /branches/experimental/toiger/src/codegen-ia32.cc (r788)
===============================================================================
Line 206: // Loads ecx with context; used below in RecordWrite.
-------------------------------------------------------------------------------
Out of date comment: ecx -> edx
File: /branches/experimental/toiger/src/virtual-frame-ia32.cc (r788)
===============================================================================
Line 75: void VirtualFrame::Adjust(int count) {
-------------------------------------------------------------------------------
The names Adjust and Forget are not symmetrical. Adjust to me sounds like
it will take both positive and negative values. I have no better name at
the moment because Remember as the opposite of Forget does not work very
well either.
Line 102: void VirtualFrame::SpillAll() {
-------------------------------------------------------------------------------
Comments please.
Line 125: void VirtualFrame::PrepareForCall(int frame_arg_count) {
-------------------------------------------------------------------------------
ditto
Line 130: // the arguments are flushed to memory.
-------------------------------------------------------------------------------
In theory you only need to materialize the values needed for the call. If
for example you have code as follows: 1 + foo(2, 3) then you would
unnecessarily push the 1 onto the stack here in advance of the call to foo.
But be aware that you must not cause holes in the stack for GC reasons, so
in the above example the values 2 and 3 should be flush against the other
values already on the stack.
Line 147: SpillAll();
-------------------------------------------------------------------------------
Wouldn't you prefer to pass a type or type mask to SpillAll()? In this case
you are only interested in spilling all constants. Maybe I am thinking too
far ahead here.
File: /branches/experimental/toiger/src/virtual-frame-ia32.h (r788)
===============================================================================
Line 81: int type_;
-------------------------------------------------------------------------------
Why not use the BitField template here? It really is a bit field and then
you do not have to encode all the operations by hand and you do not need to
space the Type values apart according to their shift value.
Line 237: int stack_pointer_;
-------------------------------------------------------------------------------
Wasn't the stack_pointer just removed a couple of changes back? Or is this
something else? Such as the actual HW sp. At the moment they should not
differ because the only operations are EmitPush/EmitPop which do force the
value onto the HW stack, correct?
Respond to these comments at http://code.google.com/p/v8/source/detail?r=788
--
You received this message because you starred this review, or because
your project has directed all notifications to a mailing list that you
subscribe to.
You may adjust your review notification preferences at:
http://code.google.com/hosting/settings
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
