Thanks a lot for comments, Mads.
http://codereview.chromium.org/669061/diff/1009/16 File src/bootstrapper.cc (right): http://codereview.chromium.org/669061/diff/1009/16#newcode1465 src/bootstrapper.cc:1465: if (function_data->IsUndefined()) { On 2010/03/08 12:39:28, Mads Ager wrote:
I wonder if this can have interesting side-effects?
Using function template info for this new pointer means that this
function will
be seen as an API function (because the function_data field will be
different
from undefined). We need to make sure that this is ok. Even if it is
ok I
think it would be nice to find a less intrusive way to do this. It is
confusing
that function_data can be different from undefined for two very
different
reasons now.
Mads, any recommendations are most appreciated. I put it into the template for the only reason---I didn't want to make all functions to pay additional pointer overhead. If you could recommend me some other place, that'd be cool. One, but somewhat nasty approach, might be to overload function_data to hold either API stuff or custom call generator, but that is somewhat smelly. http://codereview.chromium.org/669061/diff/1009/19 File src/ia32/macro-assembler-ia32.cc (right): http://codereview.chromium.org/669061/diff/1009/19#newcode171 src/ia32/macro-assembler-ia32.cc:171: lea(dst, Operand(object, dst, times_half_pointer_size, On 2010/03/08 12:39:28, Mads Ager wrote:
On 2010/03/04 21:53:53, antonm wrote: > btw, this dst (in Operand) seems mildly suspicious to me as it's not > initialized.
As discussed offline, this is indeed a bit strange. However, if
|offset| is zero
|dst| is the array index so this is the intended behavior.
Thanks a lot, Mads. http://codereview.chromium.org/669061/diff/1009/20 File src/ia32/macro-assembler-ia32.h (right): http://codereview.chromium.org/669061/diff/1009/20#newcode57 src/ia32/macro-assembler-ia32.h:57: // scratch can be object itself, but it'd get clobbered. On 2010/03/08 12:39:28, Mads Ager wrote:
it'd get -> it will be?
Done. http://codereview.chromium.org/669061/diff/1009/21 File src/ia32/stub-cache-ia32.cc (right): http://codereview.chromium.org/669061/diff/1009/21#newcode1268 src/ia32/stub-cache-ia32.cc:1268: for (int i = 0; i < argc; i++) { On 2010/03/08 12:39:28, Mads Ager wrote:
We should put a limit on the argc for which we unroll the loop here to
control
code size.
Sure. For now I am just calling a builtin if there are more than 5 args, hopefully it's fine. I would add a test for this case when we're closer to finalizing this CL. http://codereview.chromium.org/669061/diff/1009/21#newcode1286 src/ia32/stub-cache-ia32.cc:1286: for (int i = 0; i < argc; i++) { On 2010/03/08 12:39:28, Mads Ager wrote:
We should put a limit on argc for which we unroll the loop here to
control code
size. So, if argc is less than say 4 we unroll otherwise we generate
code for a
loop to do this.
Hopefully answered before. http://codereview.chromium.org/669061/diff/1009/21#newcode1291 src/ia32/stub-cache-ia32.cc:1291: __ push(edi); On 2010/03/08 12:39:28, Mads Ager wrote:
This works well for one argument, but introduces a lot of pushes and
pops for
more arguments. We should be able to improve that by freeing some
more
registers before entering the loop so the write barrier code clobbers
other
registers instead.
I'll try to do it. Mads, is there somewhere a list of registers I can use at this point without saving them? Overall, though, it might be more robust to go into builtin for this case to keep generated code as simple as possible.
It is not clear to me why we need to preserve ebx here?
Alas, currently RecordWrite clobbers object (it stores page into it, see ln. 58 (57 of reworked) macro-assembler-ia32.cc http://codereview.chromium.org/669061 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
