A few drive-by comments, this is not a real review. (I did not look at the
gory
details of Math.pow, for instance.)
* Identifier names are too cryptic (powi?), in some cases misleading
(x_is_double ==> x_is_not_smi). That makes the code harder to understand
than
it needs to be.
* Control flow with raw labels is only safe if all virtual frames reaching
the
label are identical.
* Even for the inline runtime functions where we control the only call
site, the
generated code should not rely on the specific JS code at the call site (eg,
reevaluating the arguments because you know they are parameters and thus
have no
side effects).
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.
I'm still concerned that it hasn't been carefully reviewed.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5824
src/ia32/codegen-ia32.cc:5824: Load(args->at(0));
Since the args need to be evaluated in order in all cases, please lift
it out of the conditional.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5828
src/ia32/codegen-ia32.cc:5828: Result p = allocator()->Allocate(eax);
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.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5830
src/ia32/codegen-ia32.cc:5830: Result x= frame_->Pop();
Space between 'x' and '='.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5831
src/ia32/codegen-ia32.cc:5831: if (p.is_valid() && p.reg().is(eax)) {
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.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5849
src/ia32/codegen-ia32.cc:5849: __ j(not_zero, &x_is_double);
'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".)
http://codereview.chromium.org/661179/diff/1005/1006#newcode5867
src/ia32/codegen-ia32.cc:5867: __ mov(x.reg(), y.reg());
Nothing guarantees that x and y are different registers.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5869
src/ia32/codegen-ia32.cc:5869:
Extra blank line?
http://codereview.chromium.org/661179/diff/1005/1006#newcode5977
src/ia32/codegen-ia32.cc:5977: Load(args->at(0));
This is clearly wrong---the args should not be reevaluated.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5979
src/ia32/codegen-ia32.cc:5979:
frame_->CallRuntime(Runtime::kMath_pow_cfunction, 2);
The call to CallRuntime gave you a result that represents eax. Don't
ignore this return value.
Result answer = frame()->CallRuntime(...);
http://codereview.chromium.org/661179/diff/1005/1006#newcode5983
src/ia32/codegen-ia32.cc:5983: __ bind(&return_preg);
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.
http://codereview.chromium.org/661179/diff/1005/1006#newcode5984
src/ia32/codegen-ia32.cc:5984: frame_->Push(eax);
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();
http://codereview.chromium.org/661179/diff/1005/1006#newcode5986
src/ia32/codegen-ia32.cc:5986: Load(args->at(0));
Indentation is off.
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
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.
http://codereview.chromium.org/661179/diff/1005/1006#newcode6028
src/ia32/codegen-ia32.cc:6028: Factory::heap_number_map());
Indentation is off.
http://codereview.chromium.org/661179/diff/1005/1006#newcode6034
src/ia32/codegen-ia32.cc:6034: Result scratch = allocator()->Allocate();
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.)
http://codereview.chromium.org/661179
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
