Allow the optimizing code generator to call Math.pow with untagged doubles.

Allow the optimizing code generator to call Math.pow with untagged doubles.

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

[William Hesse, 2010-12-07 16:45:43]

Additions still needed for compilation on ARM and x64.

[Kevin Millikin (Chromium), 2010-12-08 10:01:45]

Quick drive by.

http://codereview.chromium.org/5640004/diff/10001/src/assembler.cc
File src/assembler.cc (right):

http://codereview.chromium.org/5640004/diff/10001/src/assembler.cc#newcode881
src/assembler.cc:881: case Token::BIT_XOR:
Don't repurpose these tokens this way, it's a bug waiting to happen.

Introduce an enumeration in ExternalReference for the operation (and a function
to convert token to operation for the one call site in ARM that needs it).

http://codereview.chromium.org/5640004/diff/10001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5640004/diff/10001/src/hydrogen.cc#newcode4108
src/hydrogen.cc:4108: PushAndAdd(graph_->GetConstant1()->
I don't understand why you push this on the bailout environment.

http://codereview.chromium.org/5640004/diff/10001/src/hydrogen.cc#newcode4109
src/hydrogen.cc:4109: CopyToRepresentation(Representation::Double()));
This seems too roundabout.  Is there a reason not to write the more
straightforward:

AddInstruction(new HConstant(Handle<Object>(Smi::FromInt(1)),
                             Representation::Double()));

(or whatever it takes to typecheck)?

http://codereview.chromium.org/5640004/diff/10001/src/hydrogen.cc#newcode4110
src/hydrogen.cc:4110: PushAndAdd(new HUnaryMathOperation(left, kMathPowHalf));
Nor this.

[fschneider, 2010-12-08 11:27:07]

http://codereview.chromium.org/5640004/diff/10001/src/assembler.cc
File src/assembler.cc (right):

http://codereview.chromium.org/5640004/diff/10001/src/assembler.cc#newcode801
src/assembler.cc:801: 
It would be good to not duplicate code between here and runtime.cc.

http://codereview.chromium.org/5640004/diff/10001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/5640004/diff/10001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:2199: virtual bool EmitAtUses() const { return
uses()->length() <= 1; }
I don't think you need to override this here. We only use it for constants and
compare-operations.

http://codereview.chromium.org/5640004/diff/10001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/5640004/diff/10001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2198: __
CallCFunction(ExternalReference::double_fp_operation(Token::BIT_XOR), 4);
I'd write this as ExternalReference::pow()

http://codereview.chromium.org/5640004/diff/10001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2203: __
CallCFunction(ExternalReference::double_fp_operation(Token::BIT_OR), 4);
and here sth like ExternalReference::powi(), not need to use Tokens here.

http://codereview.chromium.org/5640004/diff/10001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2205:
ASSERT(instr->exponent_type().IsTagged());
Maybe you can re-use the NumberUntagD instruction here and generate two
instructions when generating the lithium-IR.

[William Hesse, 2010-12-08 13:37:00]

Suggested changes made.

http://codereview.chromium.org/5640004/diff/10001/src/assembler.cc
File src/assembler.cc (right):

http://codereview.chromium.org/5640004/diff/10001/src/assembler.cc#newcode801
src/assembler.cc:801: 
On 2010/12/08 11:27:07, fschneider wrote:
> It would be good to not duplicate code between here and runtime.cc.

Done.

http://codereview.chromium.org/5640004/diff/10001/src/assembler.cc#newcode881
src/assembler.cc:881: case Token::BIT_XOR:
I removed the power case.  Did not change the existing code.

On 2010/12/08 10:01:46, Kevin Millikin wrote:
> Don't repurpose these tokens this way, it's a bug waiting to happen.
> 
> Introduce an enumeration in ExternalReference for the operation (and a
function
> to convert token to operation for the one call site in ARM that needs it).

Done.

http://codereview.chromium.org/5640004/diff/10001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/5640004/diff/10001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:2199: virtual bool EmitAtUses() const { return
uses()->length() <= 1; }
On 2010/12/08 11:27:07, fschneider wrote:
> I don't think you need to override this here. We only use it for constants and
> compare-operations.

Done.

http://codereview.chromium.org/5640004/diff/10001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/5640004/diff/10001/src/hydrogen.cc#newcode4108
src/hydrogen.cc:4108: PushAndAdd(graph_->GetConstant1()->
On 2010/12/08 10:01:46, Kevin Millikin wrote:
> I don't understand why you push this on the bailout environment.

Done.

http://codereview.chromium.org/5640004/diff/10001/src/hydrogen.cc#newcode4109
src/hydrogen.cc:4109: CopyToRepresentation(Representation::Double()));
On 2010/12/08 10:01:46, Kevin Millikin wrote:
> This seems too roundabout.  Is there a reason not to write the more
> straightforward:
> 
> AddInstruction(new HConstant(Handle<Object>(Smi::FromInt(1)),
>                              Representation::Double()));
> 
> (or whatever it takes to typecheck)?

Done.

http://codereview.chromium.org/5640004/diff/10001/src/hydrogen.cc#newcode4110
src/hydrogen.cc:4110: PushAndAdd(new HUnaryMathOperation(left, kMathPowHalf));
On 2010/12/08 10:01:46, Kevin Millikin wrote:
> Nor this.

Done.

http://codereview.chromium.org/5640004/diff/10001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/5640004/diff/10001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:2205:
ASSERT(instr->exponent_type().IsTagged());
On 2010/12/08 11:27:07, fschneider wrote:
> Maybe you can re-use the NumberUntagD instruction here and generate two
> instructions when generating the lithium-IR.

We call two different MathPow functions here, one for integer and one for
double.

[Florian Schneider, 2010-12-08 14:09:27]

LGTM with comments below.

http://codereview.chromium.org/5640004/diff/24001/src/assembler.cc
File src/assembler.cc (right):

http://codereview.chromium.org/5640004/diff/24001/src/assembler.cc#newcode808
src/assembler.cc:808: double m = (y < 0) ? 1/x : x;
--> 1 / x

http://codereview.chromium.org/5640004/diff/24001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/5640004/diff/24001/src/hydrogen-instructions.h...
src/hydrogen-instructions.h:2199: virtual bool EmitAtUses() const { return
uses()->length() <= 1; }
Remove.

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1578: //    Abort("Tagged expoinent in Math.pow not yet
workign.");
Remove commented code?

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1586: return MarkAsCall(DefineFixedDouble(result,
xmm1), instr);
In case of a tagged exponent you need to pass CAN_DEOPTIMIZE_EAGERLY to
MarkAsCall.

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:144: //     LUnknownOSRValue
Accidental edit? UnknownOSRValue is not a UnaryOperation.

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:233: V(Power)                               \
Align \

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:1165: Representation exponent_type() { return
exponent_type_; }
You can save the exponent_type_ member here and get the representation of the
expononent from the H-instruction via the hydrogen()-accessor 

return hydrogen()->right()->representation()

and by declaring: 

DECLARE_HYDROGEN_ACCESSOR(Power)

http://codereview.chromium.org/5640004/diff/24001/src/serialize.cc
File src/serialize.cc (right):

http://codereview.chromium.org/5640004/diff/24001/src/serialize.cc#newcode426
src/serialize.cc:426:
Add(ExternalReference::double_fp_operation(Token::BIT_XOR).address(),
This can go away now.

[William Hesse, 2010-12-08 14:53:16]

http://codereview.chromium.org/5640004/diff/24001/src/assembler.cc
File src/assembler.cc (right):

http://codereview.chromium.org/5640004/diff/24001/src/assembler.cc#newcode808
src/assembler.cc:808: double m = (y < 0) ? 1/x : x;
On 2010/12/08 14:09:27, fschneider1 wrote:
> --> 1 / x

Done.

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1578: //    Abort("Tagged expoinent in Math.pow not yet
workign.");
On 2010/12/08 14:09:27, fschneider1 wrote:
> Remove commented code?

Done.

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:1586: return MarkAsCall(DefineFixedDouble(result,
xmm1), instr);
On 2010/12/08 14:09:27, fschneider1 wrote:
> In case of a tagged exponent you need to pass CAN_DEOPTIMIZE_EAGERLY to
> MarkAsCall.
>  

Done.

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:144: //     LUnknownOSRValue
On 2010/12/08 14:09:27, fschneider1 wrote:
> Accidental edit? UnknownOSRValue is not a UnaryOperation.

Done.

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:233: V(Power)                               \
On 2010/12/08 14:09:27, fschneider1 wrote:
> Align \

Done.

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:1165: Representation exponent_type() { return
exponent_type_; }
On 2010/12/08 14:09:27, fschneider1 wrote:
> You can save the exponent_type_ member here and get the representation of the
> expononent from the H-instruction via the hydrogen()-accessor 
> 
> return hydrogen()->right()->representation()
> 
> and by declaring: 
> 
> DECLARE_HYDROGEN_ACCESSOR(Power)

Done.

http://codereview.chromium.org/5640004/diff/24001/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:1165: Representation exponent_type() { return
exponent_type_; }
On 2010/12/08 14:09:27, fschneider1 wrote:
> You can save the exponent_type_ member here and get the representation of the
> expononent from the H-instruction via the hydrogen()-accessor 
> 
> return hydrogen()->right()->representation()
> 
> and by declaring: 
> 
> DECLARE_HYDROGEN_ACCESSOR(Power)

Done.