LGTM
http://codereview.chromium.org/955001/diff/3001/4001 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/955001/diff/3001/4001#newcode1445 src/ia32/codegen-ia32.cc:1445: right->number_info().IsSmi()) { What about the case where left->number_info().IsSmi()? Could you skip some of the code in that case? http://codereview.chromium.org/955001/diff/3001/4001#newcode1474 src/ia32/codegen-ia32.cc:1474: __ SmiUntag(answer.reg()); Would it be worth not untagging if doing right shifts, and just increase the shift amount? Probably not (you would have to test the increment of ecx for overflowing). http://codereview.chromium.org/955001/diff/3001/4001#newcode1514 src/ia32/codegen-ia32.cc:1514: __ test(answer.reg(), Immediate(0xc0000000)); This test is correct for SHR, but overly restrictive for the other shifts (those with signed 32-bit results). For those do: __ cmp(answer.reg(), Immediate(0xc0000000)); __ deferred->Branch(negative); Using test instead would miss the negative smi results. Also, please do a STATIC_ASSERT(kSmiValueSize == 31) to document what you are testing for (which are pretty likely for both ASR and SHL). http://codereview.chromium.org/955001/diff/3001/4001#newcode1533 src/ia32/codegen-ia32.cc:1533: } You are duplicating code from this branch. Maybe move your code inside this branch with an inner if (has SSS2 and right is smi) There are a lot of different cases that boil down to pretty much the same code: Either number can be a smi (known or not), heap number or non-number. You can convert smis and heap-numbers to int32, and the code from there is pretty much identical (except how you restore the numbers if smi-ops fail). http://codereview.chromium.org/955001 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
