It looks pretty good. I'd like to see the revision.
http://codereview.chromium.org/17380/diff/5/6 File src/codegen-ia32.cc (right): http://codereview.chromium.org/17380/diff/5/6#newcode4850 Line 4850: { Not sure why there's a block here? http://codereview.chromium.org/17380/diff/5/6#newcode4876 Line 4876: Label non_zero_product_div; The result of division is not a "product" :) http://codereview.chromium.org/17380/diff/5/6#newcode4894 Line 4894: __ lea(reg_eax.reg(), Operand(eax, eax, times_1, kSmiTag)); Isn't this temp.reg()? http://codereview.chromium.org/17380/diff/5/6#newcode4910 Line 4910: Label non_zero_product_mod; The result of mod, whatever it's called, is probably not a "product". http://codereview.chromium.org/17380/diff/5/6#newcode4918 Line 4918: __ mov(reg_eax.reg(), Operand(reg_edx.reg())); Shouldn't this be temp.reg() instead of reg_eax.reg()? http://codereview.chromium.org/17380/diff/5/6#newcode4936 Line 4936: // Move right into ecx. Comment explaining why (because we will use a shift by cl encoding). http://codereview.chromium.org/17380/diff/5/6#newcode4973 Line 4973: __ sar(ecx, kSmiTagSize); Why not right.reg()? http://codereview.chromium.org/17380/diff/5/6#newcode4993 Line 4993: JumpTarget shr_result_ok(generator()); Is it safe to make this a raw Label, because there is no frame effect that needs to be merged on either path? Otherwise, you will need a temp.ToRegister for safety below just before the lea instruction. http://codereview.chromium.org/17380/diff/5/6#newcode5009 Line 5009: Result smi_test_reg = generator()->allocator()->Allocate(); Throw in an assert that this register is valid for safety. http://codereview.chromium.org/17380 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
