The size of compiled code does not seem to change much on the V8 benchmarks, and compilation time does not seem to change on Parsy.
http://codereview.chromium.org/42006/diff/1/3 File src/codegen-ia32.cc (right): http://codereview.chromium.org/42006/diff/1/3#newcode686 Line 686: // should be inlined, placed in the stub, or omitted entirely. On 2009/03/10 10:14:47, Kevin Millikin wrote: > To parallel the order of the enum, "should be placed in the stub, inlined, or > omitted entirely". Done. http://codereview.chromium.org/42006/diff/1/3#newcode722 Line 722: class OpBits: public BitField<Token::Value, 2, 12> {}; On 2009/03/10 10:14:47, Kevin Millikin wrote: > You might consider asserting in the constructor that NUM_TOKENS fits in 12 bits. This assertion is done in BitField::encode(), which is called by MinorKey(), which I think is sufficient. http://codereview.chromium.org/42006/diff/1/3#newcode729 Line 729: ModeBits::encode(mode_) | On 2009/03/10 14:27:06, iposva wrote: > I know this is not yours, but this looks like broken indentation. Thanks! Done. http://codereview.chromium.org/42006/diff/1/3#newcode834 Line 834: // Compute the result, and return that as a constant on the frame. On 2009/03/10 10:14:47, Kevin Millikin wrote: > Maybe should avoid words like "return" because they can be ambiguous (return > from this C++ function? return at runtime?). "Compute the result and leave it > on the frame."? Done. http://codereview.chromium.org/42006/diff/1/3#newcode845 Line 845: false, overwrite_mode); On 2009/03/10 10:14:47, Kevin Millikin wrote: > Wrong indentation? Done. http://codereview.chromium.org/42006/diff/1/3#newcode848 Line 848: ConstantSmiBinaryOperation(op, &right, left.handle(), type, On 2009/03/10 10:14:47, Kevin Millikin wrote: > Wrong indentation? Done. http://codereview.chromium.org/42006/diff/1/3#newcode860 Line 860: // The same stub is used for NO_SMI_CODE and SMI_CODE_INLINED. On 2009/03/10 10:14:47, Kevin Millikin wrote: > It's probably better to make the stub handle all three values of the enum rather > than just two. It's less confusing here and will be probably be easier to > change later by just changing the stub. Done. http://codereview.chromium.org/42006/diff/1/3#newcode956 Line 956: // Create a new deferred code object that calls GenericBinaryOpStub On 2009/03/10 10:14:47, Kevin Millikin wrote: > It calls a (specialized) instance of a GenericBinaryOpStub, not the > GenericBinaryOpStub. (But I don't think you even need to mention that here, > because this non-local comment becomes out of date if the deferred code > changes). Done. http://codereview.chromium.org/42006/diff/1/3#newcode1177 Line 1177: // Consumes the argument "operand". On 2009/03/10 10:14:47, Kevin Millikin wrote: > You might consider asserting valid entry registers here, because this function > assumes that register allocation will not fail. We can't do that because we have a register used by the Result operand. http://codereview.chromium.org/42006/diff/1/3#newcode1220 Line 1220: Result answer(this); // Only allocated a new register if reversed. On 2009/03/10 10:14:47, Kevin Millikin wrote: > "allocated" -> "allocate" Done. http://codereview.chromium.org/42006/diff/1/3#newcode5608 Line 5608: // registers). On 2009/03/10 10:14:47, Kevin Millikin wrote: > You might consider asserting valid entry registers here. Left and right might be in registers, so we may have registers held by results here. http://codereview.chromium.org/42006/diff/1/3#newcode5620 Line 5620: __ or_(answer.reg(), Operand(right->reg())); On 2009/03/10 10:14:47, Kevin Millikin wrote: > (As a future change, we could simplify the smi check code if left and right are > the same registers, eg, "x + x" or "y = x; x + y".) Yes, we could. http://codereview.chromium.org/42006/diff/1/2 File src/codegen-ia32.h (right): http://codereview.chromium.org/42006/diff/1/2#newcode486 Line 486: // Emit code to perform a binary operation on On 2009/03/10 10:14:47, Kevin Millikin wrote: > Blank line between functions in the header file, please. Done. http://codereview.chromium.org/42006/diff/1/2#newcode496 Line 496: // two likely smis. The code to handle smi arguments is produced inline. On 2009/03/10 10:14:47, Kevin Millikin wrote: > Weird word wrapping (very short line followed by very long line, makes me wonder > why). For consistency, put variable names in double quotes always or never (I > don't think they need it). Done. http://codereview.chromium.org/42006 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
