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-instructions.cc
File src/hydrogen-instructions.cc (right):
https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructions.cc#newcode2839
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-instructions.cc#newcode2882
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-instructions.cc#newcode2922
src/hydrogen-instructions.cc:2922: if (op == kMathMin) {
nit: double space
https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen-instructions.h#newcode2372
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-instructions.h#newcode2405
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#newcode7795
src/hydrogen.cc:7795: case kMathFloor:
good catch!
https://chromiumcodereview.appspot.com/12315005/diff/1/src/hydrogen.cc#newcode7807
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#newcode7900
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-constants.js
File test/mjsunit/fold-constants.js (right):
https://chromiumcodereview.appspot.com/12315005/diff/1/test/mjsunit/fold-constants.js#newcode29
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-constants.js#newcode35
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-constants.js#newcode257
test/mjsunit/fold-constants.js:257:
git/patch complains about this empty line here.
https://chromiumcodereview.appspot.com/12315005/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
