Kevin,
I created a new version of Math.pow with the suggestions that you gave
(here and
offline). In addition, I changed Math.sqrt to also use a jumptarget and
copy the
frame to allow for correct handling of allocation failures.
Could you have another look?
http://codereview.chromium.org/661179/diff/1005/1006
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/661179/diff/1005/1006#newcode5817
src/ia32/codegen-ia32.cc:5817:
On 2010/02/26 13:49:33, Lasse Reichstein wrote:
I assume this is just a move, with no changes in the function.
Yes
http://codereview.chromium.org/661179/diff/1005/1006#newcode5817
src/ia32/codegen-ia32.cc:5817:
On 2010/02/28 04:56:01, Kevin Millikin wrote:
On 2010/02/26 13:49:33, Lasse Reichstein wrote:
> I assume this is just a move, with no changes in the function.
I'm still concerned that it hasn't been carefully reviewed.
I will address all of the below
http://codereview.chromium.org/661179/diff/1005/1006#newcode5824
src/ia32/codegen-ia32.cc:5824: Load(args->at(0));
On 2010/02/28 04:56:01, Kevin Millikin wrote:
Since the args need to be evaluated in order in all cases, please lift
it out of
the conditional.
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5828
src/ia32/codegen-ia32.cc:5828: Result p = allocator()->Allocate(eax);
On 2010/02/28 04:56:01, Kevin Millikin wrote:
Is there any reason to prefer eax here? I don't see one, but there is
a lot of
code.
The style guide suggests avoiding abbreviations unless they are common
in the
domain. I'm not sure 'p' counts as a common abbreviation. There must
be a
better name.
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5830
src/ia32/codegen-ia32.cc:5830: Result x= frame_->Pop();
On 2010/02/28 04:56:01, Kevin Millikin wrote:
Space between 'x' and '='.
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5831
src/ia32/codegen-ia32.cc:5831: if (p.is_valid() && p.reg().is(eax)) {
On 2010/02/28 04:56:01, Kevin Millikin wrote:
p is always valid. You've allocated eax with no live values, so it
must be.
It cannot be other than eax. The register allocator will not give you
some
other register when you request eax.
No need for the if, a simple ASSERT works if you're worried about
something.
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5849
src/ia32/codegen-ia32.cc:5849: __ j(not_zero, &x_is_double);
On 2010/02/28 04:56:01, Kevin Millikin wrote:
'x_is_double' seems like a bad name, since we're not sure it's a
double. (Plus,
we usually use the term "heap number".)
Changed
http://codereview.chromium.org/661179/diff/1005/1006#newcode5867
src/ia32/codegen-ia32.cc:5867: __ mov(x.reg(), y.reg());
On 2010/02/28 04:56:01, Kevin Millikin wrote:
Nothing guarantees that x and y are different registers.
As discussed offline - we are actually sure that they are heap numbers
and that they are different.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5869
src/ia32/codegen-ia32.cc:5869:
On 2010/02/28 04:56:01, Kevin Millikin wrote:
Extra blank line?
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5977
src/ia32/codegen-ia32.cc:5977: Load(args->at(0));
On 2010/02/28 04:56:01, Kevin Millikin wrote:
This is clearly wrong---the args should not be reevaluated.
As discussed offline this has now been changed completely.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5979
src/ia32/codegen-ia32.cc:5979:
frame_->CallRuntime(Runtime::kMath_pow_cfunction, 2);
On 2010/02/28 04:56:01, Kevin Millikin wrote:
The call to CallRuntime gave you a result that represents eax. Don't
ignore
this return value.
Result answer = frame()->CallRuntime(...);
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5983
src/ia32/codegen-ia32.cc:5983: __ bind(&return_preg);
On 2010/02/28 04:56:01, Kevin Millikin wrote:
There are different live values on the two paths to this label.
That's bad.
1. Unuse x and y before jumping here. They are not really live.
2. Use JumpTarget for control flow to allow the register allocator to
merge live
values in potentially different registers.
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5984
src/ia32/codegen-ia32.cc:5984: frame_->Push(eax);
On 2010/02/28 04:56:01, Kevin Millikin wrote:
Avoid pushing registers directly. You can choose to explicitly pass
the live
value along both branches:
JumpTarget done;
x.Unuse();
y.Unuse();
done.Jump(&p);
...
done.Bind(&answer);
frame()->Push(&answer);
Or else implicitly as the top frame element:
JumpTarget done;
x.Unuse();
y.Unuse();
frame()->Push(&p);
done.Jump();
...
Result answer = frame()->CallRuntime(...);
frame()->Push(&answer);
done.Bind();
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5986
src/ia32/codegen-ia32.cc:5986: Load(args->at(0));
On 2010/02/28 04:56:01, Kevin Millikin wrote:
Indentation is off.
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode6015
src/ia32/codegen-ia32.cc:6015: Result result = frame_->Pop();
On 2010/02/26 13:49:33, Lasse Reichstein wrote:
Remember to do result.ToRegister() and result.Spill().
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode6025
src/ia32/codegen-ia32.cc:6025: // We should never have anything but a
double here - in case we
On 2010/02/28 04:56:01, Kevin Millikin wrote:
I don't understand the comment because I don't understand the context.
But in
general:
1. If this check could be expected to fail, you have to do something
safe (even
crashing via CHECK) instead of this. As it stands, there are two
different
frame heights reaching label end, and then who knows what will happen?
2. If this is a precondition that the caller should guarantee, then
don't do the
comparison in release builds. Instead, generate debug code guarded by
if
(FLAG_debug_code) to verify the precondition and assert.
As discussed offline we always have heapnumbers here - I have added a
check in case the FLAG_debug_code flag is set.
http://codereview.chromium.org/661179/diff/1005/1006#newcode6028
src/ia32/codegen-ia32.cc:6028: Factory::heap_number_map());
On 2010/02/28 04:56:01, Kevin Millikin wrote:
Indentation is off.
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode6029
src/ia32/codegen-ia32.cc:6029: __ j(not_equal, &end);
On 2010/02/26 13:49:33, Lasse Reichstein wrote:
Move frame_->Push(&result) after bind(&end) so it becomes obvious that
we return
result.reg() in either case.
(That's what happens now anyway, it's hard to see).
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode6034
src/ia32/codegen-ia32.cc:6034: Result scratch = allocator()->Allocate();
On 2010/02/28 04:56:01, Kevin Millikin wrote:
Using plain labels instead of JumpTarget for control flow is only safe
if there
is no frame effect.
Allocating a register is a frame effect, so the frames reaching end
not only
have different heights, but they could differ in an arbitrary element.
(They
actually won't, because all platforms have more than one register
available for
allocation, but you should not rely on that.)
Done.
http://codereview.chromium.org/661179/diff/1005/1006#newcode6035
src/ia32/codegen-ia32.cc:6035: __ AllocateHeapNumber(result.reg(),
scratch.reg(), no_reg, &end);
On 2010/02/26 13:49:33, Lasse Reichstein wrote:
If allocation fails, result.reg() have been changed to point to
unallocated
memory (very near the end of the newspace semispace). On failure you
should go
to somewhere that calls runtime and does finishes the job instead of
just
returning and (eventually) crashing.
Done.
http://codereview.chromium.org/661179
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
