I have not looked in detail at the register reshuffling due to the use of ecx as backtrack stack pointer.
Set of comments below after which it should look good to me. http://codereview.chromium.org/17416/diff/28/232 File src/regexp-macro-assembler-ia32.cc (left): http://codereview.chromium.org/17416/diff/28/232#oldcode144 Line 144: __ mov(eax, current_character()); Thanks! http://codereview.chromium.org/17416/diff/28/232 File src/regexp-macro-assembler-ia32.cc (right): http://codereview.chromium.org/17416/diff/28/232#newcode259 Line 259: // Store current state before reusing the registers. Should we add a comment like this? (Not sure if I mentioned all the right registers.) // After these pushes the registers eax, ebx, ecx, edx and edi are available in the code below. http://codereview.chromium.org/17416/diff/28/232#newcode309 Line 309: // Store registers before calling C function. Store registers -> Save registers http://codereview.chromium.org/17416/diff/28/232#newcode627 Line 627: __ mov(ecx, Operand(ebp, kStackHighEnd)); How about using backtrack_stackpointer() instead of ecx here? http://codereview.chromium.org/17416/diff/28/232#newcode708 Line 708: __ bind(&stack_overflow); Can you please comment why it is still possible to reach here to distinguish from the stack overflow handling below? If I am not mistaken the preemption check does verify that the native stack is sane when popping backtracking information, so if too much things were pushed on the native stack you would jump to the preemption label and the code determines that this was not really a preemption but a native stack overflow => return with an exception. Correct? http://codereview.chromium.org/17416/diff/28/232#newcode738 Line 738: __ bind(&grow_failed); You could consolidate this label with the stack_overflow label from above and name it exit_with_exception. http://codereview.chromium.org/17416/diff/28/232#newcode987 Line 987: Register RegExpMacroAssemblerIA32::backtrack_stackpointer() { How about defining this function straight in the header file. As well as other accessor like functions such as PushCurrentPosition, ... http://codereview.chromium.org/17416/diff/28/232#newcode1043 Line 1043: Operand(backtrack_stackpointer(), -kPointerSize)); Why not simplify and always use this pattern regardless of the register being pushed? http://codereview.chromium.org/17416/diff/28/232#newcode1057 Line 1057: __ mov(Operand(backtrack_stackpointer(), 0), value); If you do decide to only use one pattern above, you might want to turn this one around for consistency. http://codereview.chromium.org/17416/diff/28/236 File src/regexp-stack.cc (right): http://codereview.chromium.org/17416/diff/28/236#newcode35 Line 35: ExecutionAccess lock; Please see comment about ExecutionAccess in the header file. I believe it applies here too. http://codereview.chromium.org/17416/diff/28/236#newcode46 Line 46: ExecutionAccess lock; ditto http://codereview.chromium.org/17416/diff/28/236#newcode54 Line 54: ExecutionAccess lock; ditto http://codereview.chromium.org/17416/diff/28/236#newcode63 Line 63: ExecutionAccess lock; ditto http://codereview.chromium.org/17416/diff/28/237 File src/regexp-stack.h (right): http://codereview.chromium.org/17416/diff/28/237#newcode44 Line 44: // Gives the top of the memory used as stack. Please use empty line between last piece of code and comment for next function declaration to add structure for the reader's eye. http://codereview.chromium.org/17416/diff/28/237#newcode46 Line 46: ExecutionAccess lock; I don't think you need to own the ExecutionAccess here since you are not calling StackGuard methods. And the external preemption thread is not going to mess with this thread_local_ data. Ask Erik Corry to be safe though. http://codereview.chromium.org/17416/diff/28/237#newcode52 Line 52: ExecutionAccess lock; ditto http://codereview.chromium.org/17416/diff/28/237#newcode60 Line 60: ExecutionAccess lock; ditto http://codereview.chromium.org/17416 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
