LGTM with a few corrections.
http://codereview.chromium.org/548179/diff/18/3005 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/548179/diff/18/3005#newcode6571 src/x64/codegen-x64.cc:6571: if (!FLAG_regexp_entry_native) { You could check up-front whether we are compiled with native regexps. I.e., #ifndef V8_NATIVE_REGEXP // call runtime always (which calls the bytecode interpreter) #endif The code still works, since the generated code will be a ByteArray, not a Code object, so we go to runtime when that is checked, but only after a lot of wasted checks. http://codereview.chromium.org/548179/diff/18/3005#newcode6605 src/x64/codegen-x64.cc:6605: __ j(is_smi, &runtime); Use JumpIfSmi(rax, &runtime); Generally, the sequence of Condition c = CheckSomething() j(c, somewhere) should have a JumpIfSomething(somewhere) shorthand (and probably also a JumpIfNotSomething(somewhere)). It's MUCH more readable. If it isn't there, do add it. Using the Check macros directly should be reserved for code that expects a condition as part of a more complicated dispatch (e.g., virtual frame functions). http://codereview.chromium.org/548179/diff/18/3005#newcode6638 src/x64/codegen-x64.cc:6638: is_smi = masm->CheckSmi(rax); JumpIfSmi http://codereview.chromium.org/548179/diff/18/3005#newcode6651 src/x64/codegen-x64.cc:6651: __ j(NegateCondition(is_smi), &runtime); You are not checking that it's positive. You can use JumpIfNotPositiveSmi http://codereview.chromium.org/548179/diff/18/3005#newcode6653 src/x64/codegen-x64.cc:6653: __ SmiCompare(rax, rbx); rbx is not a smi. You should un-smi-tag rax before comparing. Or, perhaps, make a LoadSmiToInteger32 macro that loads a smi and converts it to an int (which can be done efficiently by loading the top half of the smi). If you untag and compare as int32's, you can do unsigned comparison to fail if the value is negative. http://codereview.chromium.org/548179/diff/18/3005#newcode6670 src/x64/codegen-x64.cc:6670: __ movq(rax, FieldOperand(rbx, FixedArray::kLengthOffset)); The FixedArray::length is an int, use movl to load it. Mention that a FixedArray can't have a length so large that length+kLastMatchOverhead overflows. http://codereview.chromium.org/548179/diff/18/3005#newcode6694 src/x64/codegen-x64.cc:6694: // a sequential string. Are we sure that sequential string haven't been turned into an external string? I.e., I think it could be external. (This seems to be assumed in ia32 too). http://codereview.chromium.org/548179/diff/18/3005#newcode6705 src/x64/codegen-x64.cc:6705: __ andl(rbx, Immediate(kStringRepresentationEncodingMask)); Consider adding andb to the assembler. It should be fairly simple (I'm guessing void andb(Register dst, Immediate src) { immediate_arithmetic_op_8(0x4, dst, src); } should be sufficient, but hasn't been checked). http://codereview.chromium.org/548179/diff/18/3005#newcode6730 src/x64/codegen-x64.cc:6730: // Check that the irregexp code has been generated for If it has, the field Missing words before "If". http://codereview.chromium.org/548179/diff/18/3005#newcode6736 src/x64/codegen-x64.cc:6736: // rdi: encoding of subject string (1 if ascii 0 if two_byte); Comma before "0". http://codereview.chromium.org/548179/diff/18/3005#newcode6756 src/x64/codegen-x64.cc:6756: __ ArgumentStackSlotsForCFunctionCall(kRegExpExecuteArguments); use masm-> (or masm_-> or masm()->, whatever __ abbreviates) instead of __. When computing code coverage, the __ macro can't return values. http://codereview.chromium.org/548179/diff/18/3005#newcode6841 src/x64/codegen-x64.cc:6841: __ j(equal, &runtime); We are effectively rerunning the regexp just to get an exception. But we know that the exception is an out-of-regexp-stack exception (the only kind that is generated inside the RegExp code and doesn't set the pending exception). Could we have a runtime function that only raised that exception? http://codereview.chromium.org/548179/diff/18/3005#newcode6853 src/x64/codegen-x64.cc:6853: __ SmiToInteger64(rdx, rdx); You used SmiTimesPowerOfTwoToInteger64 above. http://codereview.chromium.org/548179/diff/18/3005#newcode6892 src/x64/codegen-x64.cc:6892: __ movq(rdi, Operand(rcx, rdx, times_int_size, 0)); movl? http://codereview.chromium.org/548179/diff/18/3005#newcode6896 src/x64/codegen-x64.cc:6896: // Carry flag set by smi convertion above. Carry flag -> Negative flag. Add it to the documentation of Integer32ToSmi that its assumed to set N and Z flags depending on the outcome (so someone reimplementing it won't break your code). http://codereview.chromium.org/548179/diff/18/3006 File src/x64/regexp-macro-assembler-x64.cc (right): http://codereview.chromium.org/548179/diff/18/3006#newcode332 src/x64/regexp-macro-assembler-x64.cc:332: // Caller save on Win64 "Callee save" is correct (but confusing, since we are not on Win64). So change to "Caller save (but not on Win64)". http://codereview.chromium.org/548179/diff/4007/5005 File src/x64/macro-assembler-x64.cc (right): http://codereview.chromium.org/548179/diff/4007/5005#newcode2494 src/x64/macro-assembler-x64.cc:2494: // Make stack end at alignment and allocate space for argument slots on stack -> "arguments slots and old rsp on stack" http://codereview.chromium.org/548179 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
