LGTM
http://codereview.chromium.org/1523030/diff/1/5 File src/arm/simulator-arm.cc (right): http://codereview.chromium.org/1523030/diff/1/5#newcode476 src/arm/simulator-arm.cc:476: There are a number of long lines. http://codereview.chromium.org/1523030/diff/1/5#newcode477 src/arm/simulator-arm.cc:477: static const int kICachePageSize = 0x1000; 0x1000 -> 1 << kICachePageShift http://codereview.chromium.org/1523030/diff/1/5#newcode478 src/arm/simulator-arm.cc:478: static const int kICachePageMask = 0xfff; 0xfff -> kICachePageSize - 1 http://codereview.chromium.org/1523030/diff/1/5#newcode481 src/arm/simulator-arm.cc:481: static const int kICacheLineMask = 3; 3 -> kICacheLineLength - 1 http://codereview.chromium.org/1523030/diff/1/5#newcode482 src/arm/simulator-arm.cc:482: static const int kPageSpace = kICachePageSize + kICachePageSize / kICacheLineLength; Please add a comment to the kPageSpace constant, and the layout of the memory allocated for a page. http://codereview.chromium.org/1523030/diff/1/5#newcode486 src/arm/simulator-arm.cc:486: ASSERT((reinterpret_cast<intptr_t>(one) & kICachePageMask) == 0); ASSERT this for two as well? http://codereview.chromium.org/1523030/diff/1/5#newcode534 src/arm/simulator-arm.cc:534: char* new_page = new char[kPageSpace]; Please consider encapsulating the byte array for representing a page in a class providing methods for manipulating and inspecting it. http://codereview.chromium.org/1523030/diff/1/5#newcode535 src/arm/simulator-arm.cc:535: memset(new_page, 1, kPageSpace); // Set the ICache to invalid for the new pages. the new -> new http://codereview.chromium.org/1523030/diff/1/5#newcode535 src/arm/simulator-arm.cc:535: memset(new_page, 1, kPageSpace); // Set the ICache to invalid for the new pages. Please add named constants for the validity of a cache line. http://codereview.chromium.org/1523030/diff/1/5#newcode537 src/arm/simulator-arm.cc:537: //printf("New cache allocated from %p to %p\n", (void*)(new_page), (void*)(new_page + kPageSpace)); Code in comments (here and below) - maybe add FLAG_trace_sim_icache? Then use PrintF instead of printf. http://codereview.chromium.org/1523030/diff/1/5#newcode555 src/arm/simulator-arm.cc:555: memset(valid_bytemap, 0, size / kICacheLineLength); Please add named constants for the validity of a cache line. http://codereview.chromium.org/1523030/diff/1/5#newcode575 src/arm/simulator-arm.cc:575: } { I think you want an else here. http://codereview.chromium.org/1523030/diff/1/5#newcode665 src/arm/simulator-arm.cc:665: assembler::arm::Simulator::current()-> Is assembler::arm:: required here? http://codereview.chromium.org/1523030/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
