Add checks to the ARM simulator to ensure that we flush the icache all...

Add checks to the ARM simulator to ensure that we flush the icache all
the places we should.

Committed: http://code.google.com/p/v8/source/detail?r=4474

[Erik Corry, 2010-04-20 07:55:29]

The current version does not work for threaded tests.  There is one simulator
per thread which results in one icache per thread which is obviously wrong as
the CPU has only one icache.

Apart from that it is ready for review and passes all tests with and without
snapshots.

[Søren Thygesen Gjesse, 2010-04-20 10:22:02]

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?

[Erik Corry, 2010-04-22 09:44:31]

Thanks for review.  A new version has been uploaded that addresses all comments
and works for threaded programs.  I had to disable cache checking on the release
version since it slowed down the ARM simulator by a factor of 4.

[Søren Thygesen Gjesse, 2010-04-22 12:54:26]

LGTM