Comments from the side to help cleanup the code... -Ivan
http://codereview.chromium.org/67163/diff/1/11 File src/codegen-arm.cc (right): http://codereview.chromium.org/67163/diff/1/11#newcode4550 Line 4550: __ ldr(r2, MemOperand(r0, HeapNumber::kValueOffset - kHeapObjectTag)); On 2009/04/15 08:01:04, Kevin Millikin wrote: > On IA32 there is a FieldOperand abstraction (in macro-assembler-ia32.h) that > does the subtract kHeapObjectTag thing. You might consider the same for ARm and > using it here. There is a FieldMemOperand(Register object, int offset) in macro-assembler-arm.cc which should be used here and in many other places in this new code. http://codereview.chromium.org/67163/diff/1/9 File src/simulator-arm.cc (right): http://codereview.chromium.org/67163/diff/1/9#newcode435 Line 435: sixty_four_bits[0] = registers_[0]; There is an accessor for this: get_register(r0) http://codereview.chromium.org/67163/diff/1/9#newcode438 Line 438: memcpy(x, buffer, sizeof(sixty_four_bits)); Can you please explain why you copy registers->sixty_four_bits->buffer->x? This seems a very round-about way of doing something like this: int32_t* xpointer = reinterpret_cast<int32_t>(x); xpointer[0] = registers_[0]; xpointer[1] = registers_[1]; If this does not work, then please explain in comments exactly why these multiple steps are necessary. http://codereview.chromium.org/67163/diff/1/9#newcode451 Line 451: registers_[0] = sixty_four_bits[0]; There is an accessor for this: set_register(r0, value). http://codereview.chromium.org/67163/diff/1/9#newcode452 Line 452: registers_[1] = sixty_four_bits[1]; Again multiple copies. Why? http://codereview.chromium.org/67163/diff/1/9#newcode456 Line 456: void Simulator::TrashCalleeSaveRegisters() { You probably mean to trash the CallerSaved registers. CalleeSavedRegisters should be preserved across the call and not trashed. Also this method seems to be not used. How about just removing it for now? http://codereview.chromium.org/67163/diff/1/9#newcode936 Line 936: GetFpArgs(&x, &y); How about making the Simulator API a bit more generic? x = GetDoubleValue(r0, r1); y = GetDoubleValue(r2, r3); http://codereview.chromium.org/67163/diff/1/9#newcode937 Line 937: double z = (swi == simulator_fp_add) ? On 2009/04/15 07:40:36, Kasper Lund wrote: > This looks nasty. Why not write it as a series of if-else-if? Yes, please! http://codereview.chromium.org/67163 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
