Constant fold math and string operations.

Constant fold math and string operations.


R=jkummerow@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=13705

[Jakob Kummerow, 2013-02-20 15:31:04]

Mostly nits, except for the observation that some of the functions in the test
case deopt.

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:2839: if (isinf(d)) {  // +Inifinity and -Infinity.
nit: "Inifinity" sounds nice, but has an "i" too many.

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:2882: if (Double(d).Exponent() >= 0) return
H_CONSTANT_DOUBLE(d);
I'm not convinced having this "fast path" is worthwhile, but whatever...

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:2922: if  (op == kMathMin) {
nit: double space

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.h:2372: HUnaryMathOperation(HValue* context, HValue*
value, BuiltinFunctionId op)
Since you're introducing a factory method below, you don't want this ctor to be
used any more, right? Please move it to the "private:" section.
Same for all other HInstructions where you introduced factory methods.

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.h:2405: static HInstruction* NewHUnaryMathOperation(
idea: You could shorten this to just "New", as it's uniquely identified by the
enclosing class anyway (call sites would be "... =
HUnaryMathOperation::New(...);"). I don't mind either way, though.

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen.cc#newcod...
src/hydrogen.cc:7795: case kMathFloor:
good catch!

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen.cc#newcod...
src/hydrogen.cc:7807: zone(), context, argument, id);
nit: I'd indent 4 spaces relative to "HInstruction*", but if you prefer this
that's fine too.

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen.cc#newcod...
src/hydrogen.cc:7900: zone(), context, left, kMathPowHalf);
nit: now this indentation is just inconsistent with everything else.

https://chromiumcodereview.appspot.com/12315005/diff/1/test/mjsunit/fold-cons...
File test/mjsunit/fold-constants.js (right):

https://chromiumcodereview.appspot.com/12315005/diff/1/test/mjsunit/fold-cons...
test/mjsunit/fold-constants.js:29: // Flags: --nodead-code-elimination
--constant-folding --allow-natives-syntax
Warning: unknown flag --constant-folding.

I think you mean --fold-constants.
Also, have you noticed that we already have test/mjsunit/constant-folding.js?
I'd suggest to rename this new test to constant-folding-2.js (as it's more
intuitive to look for some name beginning with "constant").

https://chromiumcodereview.appspot.com/12315005/diff/1/test/mjsunit/fold-cons...
test/mjsunit/fold-constants.js:35: f();
It would be nice to assertTrue(%GetOptimizationStatus(f) != 2) after this line.
However, that currently fails, which it shouldn't, since folded constants should
not deopt ;-)

https://chromiumcodereview.appspot.com/12315005/diff/1/test/mjsunit/fold-cons...
test/mjsunit/fold-constants.js:257: 
git/patch complains about this empty line here.

[Yang, 2013-02-20 17:51:15]

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:2839: if (isinf(d)) {  // +Inifinity and -Infinity.
On 2013/02/20 15:31:04, Jakob wrote:
> nit: "Inifinity" sounds nice, but has an "i" too many.

Done.

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:2882: if (Double(d).Exponent() >= 0) return
H_CONSTANT_DOUBLE(d);
On 2013/02/20 15:31:04, Jakob wrote:
> I'm not convinced having this "fast path" is worthwhile, but whatever...

This is not a fast path, but for correctness.  The rounding error may result in
1.0 being added when you try to add 0.5.

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen.cc#newcod...
src/hydrogen.cc:7900: zone(), context, left, kMathPowHalf);
On 2013/02/20 15:31:04, Jakob wrote:
> nit: now this indentation is just inconsistent with everything else.

Done.

https://chromiumcodereview.appspot.com/12315005/diff/1/test/mjsunit/fold-cons...
File test/mjsunit/fold-constants.js (right):

https://chromiumcodereview.appspot.com/12315005/diff/1/test/mjsunit/fold-cons...
test/mjsunit/fold-constants.js:257: 
On 2013/02/20 15:31:04, Jakob wrote:
> git/patch complains about this empty line here.

Done.

[Yang, 2013-02-20 17:52:18]

The issue was that -Infinity was not being constant folded, but still computed
as -1 * Infinity. Math.round of that deopts.

On 2013/02/20 17:51:15, Yang wrote:
>
https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
> File src/hydrogen-instructions.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
> src/hydrogen-instructions.cc:2839: if (isinf(d)) {  // +Inifinity and
-Infinity.
> On 2013/02/20 15:31:04, Jakob wrote:
> > nit: "Inifinity" sounds nice, but has an "i" too many.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
> src/hydrogen-instructions.cc:2882: if (Double(d).Exponent() >= 0) return
> H_CONSTANT_DOUBLE(d);
> On 2013/02/20 15:31:04, Jakob wrote:
> > I'm not convinced having this "fast path" is worthwhile, but whatever...
> 
> This is not a fast path, but for correctness.  The rounding error may result
in
> 1.0 being added when you try to add 0.5.
> 
> https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen.cc
> File src/hydrogen.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen.cc#newcod...
> src/hydrogen.cc:7900: zone(), context, left, kMathPowHalf);
> On 2013/02/20 15:31:04, Jakob wrote:
> > nit: now this indentation is just inconsistent with everything else.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/12315005/diff/1/test/mjsunit/fold-cons...
> File test/mjsunit/fold-constants.js (right):
> 
>
https://chromiumcodereview.appspot.com/12315005/diff/1/test/mjsunit/fold-cons...
> test/mjsunit/fold-constants.js:257: 
> On 2013/02/20 15:31:04, Jakob wrote:
> > git/patch complains about this empty line here.
> 
> Done.

[Yang, 2013-02-21 09:39:51]

Also renamed the factory methods.

On 2013/02/20 17:52:18, Yang wrote:
> The issue was that -Infinity was not being constant folded, but still computed
> as -1 * Infinity. Math.round of that deopts.
> 
> On 2013/02/20 17:51:15, Yang wrote:
> >
>
https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
> > File src/hydrogen-instructions.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
> > src/hydrogen-instructions.cc:2839: if (isinf(d)) {  // +Inifinity and
> -Infinity.
> > On 2013/02/20 15:31:04, Jakob wrote:
> > > nit: "Inifinity" sounds nice, but has an "i" too many.
> > 
> > Done.
> > 
> >
>
https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructi...
> > src/hydrogen-instructions.cc:2882: if (Double(d).Exponent() >= 0) return
> > H_CONSTANT_DOUBLE(d);
> > On 2013/02/20 15:31:04, Jakob wrote:
> > > I'm not convinced having this "fast path" is worthwhile, but whatever...
> > 
> > This is not a fast path, but for correctness.  The rounding error may result
> in
> > 1.0 being added when you try to add 0.5.
> > 
> > https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen.cc
> > File src/hydrogen.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen.cc#newcod...
> > src/hydrogen.cc:7900: zone(), context, left, kMathPowHalf);
> > On 2013/02/20 15:31:04, Jakob wrote:
> > > nit: now this indentation is just inconsistent with everything else.
> > 
> > Done.
> > 
> >
>
https://chromiumcodereview.appspot.com/12315005/diff/1/test/mjsunit/fold-cons...
> > File test/mjsunit/fold-constants.js (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/12315005/diff/1/test/mjsunit/fold-cons...
> > test/mjsunit/fold-constants.js:257: 
> > On 2013/02/20 15:31:04, Jakob wrote:
> > > git/patch complains about this empty line here.
> > 
> > Done.

[Jakob Kummerow, 2013-02-21 10:14:56]

LGTM with nits.

https://chromiumcodereview.appspot.com/12315005/diff/10002/src/hydrogen-instr...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/12315005/diff/10002/src/hydrogen-instr...
src/hydrogen-instructions.cc:2756: Zone* zone, HValue* context, HValue* left,
HValue* right) {                \
nit: for method/function declarations (as opposed to calls), every parameter
goes on its own line (unless everything, including the method name, fits on one
line).
Again several times below.

https://chromiumcodereview.appspot.com/12315005/diff/10002/src/hydrogen-instr...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/12315005/diff/10002/src/hydrogen-instr...
src/hydrogen-instructions.h:3898: HPower(HValue* left, HValue* right) {
private ctor please

https://chromiumcodereview.appspot.com/12315005/diff/10002/src/hydrogen-instr...
src/hydrogen-instructions.h:3954: HAdd(HValue* context, HValue* left, HValue*
right)
private ctor please

https://chromiumcodereview.appspot.com/12315005/diff/10002/src/hydrogen-instr...
src/hydrogen-instructions.h:4002: HSub(HValue* context, HValue* left, HValue*
right)
private ctor please

https://chromiumcodereview.appspot.com/12315005/diff/10002/src/hydrogen-instr...
src/hydrogen-instructions.h:5467: HStringCharFromCode(HValue* context, HValue*
char_code) {
private ctor please

https://chromiumcodereview.appspot.com/12315005/diff/10002/src/hydrogen-instr...
src/hydrogen-instructions.h:5500: explicit HStringLength(HValue* string) :
HUnaryOperation(string) {
private ctor please

[Yang, 2013-02-21 11:40:47]

Committed patchset #5 manually as r13705 (presubmit successful).