2009/4/16 <[email protected]>: > 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.
Fixed in new and old 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) See below. > 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. I removed a step by copying directly from registers_[] (which means I now can't use the accessor). The reason for the memcpy is to get around the strict aliasing rules. Explained in a new comment. > 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). See above. > http://codereview.chromium.org/67163/diff/1/9#newcode452 > Line 452: registers_[1] = sixty_four_bits[1]; > Again multiple copies. Why? See above. > 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? Renamed and now used. > > 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! Done! -- Erik Corry, Software Engineer Google Denmark ApS. CVR nr. 28 86 69 84 c/o Philip & Partners, 7 Vognmagergade, P.O. Box 2227, DK-1018 Copenhagen K, Denmark. --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
