A first set of comments. Please let me know when there is an updated version. I will try not to loose the notification in my Inbox.
-Ivan http://codereview.chromium.org/5002/diff/1/2 File src/assembler-ia32.cc (right): http://codereview.chromium.org/5002/diff/1/2#newcode2043 Line 2043: position + static_cast<int>(sizeof(uint32_t)) <= pc_offset()); I sometimes find it useful to have two ASSERT statements instead of && because the ASSERT notification will tell you which of the cases failed. Is there any particular reason for this change? http://codereview.chromium.org/5002/diff/1/3 File src/codegen-arm.cc (right): http://codereview.chromium.org/5002/diff/1/3#newcode302 Line 302: virtual int FastCaseSwitchMaxOverheadFactor(); Personally I would remove the new lines between the three declarations and add a single line comment similar to the group "Control flow" above. http://codereview.chromium.org/5002/diff/1/3#newcode2757 Line 2757: return 5; Can we use some constants instead of hard-coded numbers? The ia32 a some constant here and the constant is well documented explaining what it is being used for. http://codereview.chromium.org/5002/diff/1/3#newcode2757 Line 2757: return 5; Given that a single case check against a Smi looks as below on ARM, I think 5 is too conservative. 0x37a1d0 184 e59d0000 ldr r0, [sp, #+0] 0x37a1d4 188 e52d0004 str r0, [sp, #-4]! 0x37a1d8 192 e3a00002 mov r0, #2 0x37a1dc 196 e49d1004 ldr r1, [sp], #+4 0x37a1e0 200 e1802001 orr r2, r0, r1 0x37a1e4 204 e3120001 tst r2, #1 0x37a1e8 208 0a000009 beq +44 -> 252 (0x37a214) 0x37a1ec 212 e52d1004 str r1, [sp, #-4]! 0x37a1f0 216 e52d0004 str r0, [sp, #-4]! 0x37a1f4 220 e3a00001 mov r0, #1 0x37a1f8 224 e5981017 ldr r1, [r8, #+23] 0x37a1fc 228 e591100b ldr r1, [r1, #+11] 0x37a200 232 e5911017 ldr r1, [r1, #+23] 0x37a204 236 e1a0e00f mov lr, pc 0x37a208 240 e59ff0c4 ldr pc, [pc, #+196] ;; code: FUNCTION 0x37a20c 244 e3500000 cmp r0, #0 0x37a210 248 ea000000 b +8 -> 256 (0x37a218) 0x37a214 252 e1510000 cmp r1, r0 0x37a218 256 1a000021 bne +140 -> 396 (0x37a2a4) 0x37a21c 260 e49d0004 ldr r0, [sp], #+4 http://codereview.chromium.org/5002/diff/1/3#newcode2761 Line 2761: return 5; Same as above. Please use constant or at least document how you arrive at the magic number. http://codereview.chromium.org/5002/diff/1/3#newcode2773 Line 2773: // small positive numbers can be immediate opreands. opreands -> operands http://codereview.chromium.org/5002/diff/1/3#newcode2785 Line 2785: __ nop(); // 0ffsets table, so its address is the pc-register at the add. Maybe you could use __ bkpt() or __ stop("switch table alignment") to indicate that this instruction is not going to be executed ever. http://codereview.chromium.org/5002 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
