A64: Replace memcpy with reinterpret_cast assignment in simulator and decoder.

A64: Replace memcpy with reinterpret_cast assignment in simulator and decoder.

BUG=

[ulan, 2014-02-18 17:35:18]

PTAL. Is this safe to replace memcpy? This speeds up the simulator by ~20%:

Richards: 51.5 (before)
Richards: 63.9 (after)

[Rodolph Perfetta (ARM), 2014-02-18 18:26:05]

It is not safe, and I am surprised at the speedup, the compiler should be able
to generate very similar code with the memcpy version. At least C compilers do.
Have you tried replacing the mempcy by __builtin_memcpy, it may give a hint to
the compiler.

another idea in debug would be to compile the simulator itself in release mode:
there are a fair amount of asserts in the decoder and skipping those should
help.

On 2014/02/18 17:35:18, ulan wrote:
> PTAL. Is this safe to replace memcpy? This speeds up the simulator by ~20%:
> 
> Richards: 51.5 (before)
> Richards: 63.9 (after)

[jochen (gone - plz use gerrit), 2014-02-18 21:14:25]

On 2014/02/18 18:26:05, Rodolph Perfetta (ARM) wrote:
> It is not safe, and I am surprised at the speedup, the compiler should be able
> to generate very similar code with the memcpy version. At least C compilers
do.
> Have you tried replacing the mempcy by __builtin_memcpy, it may give a hint to
> the compiler.

Can you explain why this is not safe?

I doubt that the compiler can optimize the memcpy calls away. We profiled a
release build of d8, and more than 10% of the total runtime are spend in memcpy.

> 
> another idea in debug would be to compile the simulator itself in release
mode:
> there are a fair amount of asserts in the decoder and skipping those should
> help.
> 

The numbers Ulan gave are on a release build.

> On 2014/02/18 17:35:18, ulan wrote:
> > PTAL. Is this safe to replace memcpy? This speeds up the simulator by ~20%:
> > 
> > Richards: 51.5 (before)
> > Richards: 63.9 (after)

[jbramley, 2014-02-19 10:03:16]

On 2014/02/18 21:14:25, jochen wrote:
> On 2014/02/18 18:26:05, Rodolph Perfetta (ARM) wrote:
> > It is not safe, and I am surprised at the speedup, the compiler should be
able
> > to generate very similar code with the memcpy version. At least C compilers
> do.
> > Have you tried replacing the mempcy by __builtin_memcpy, it may give a hint
to
> > the compiler.
> 
> Can you explain why this is not safe?

Using reinterpret_cast violates type aliasing rules. In C(++), if you write to
an object of type X, then read it as type Y, the results are undefined. Usually,
things work anyway, but sometimes (with new compiler optimisations) things stop
working. That doesn't mean that you can't use reinterpret_cast, because it is
possible to use it safely, but I don't think it's possible here.

For simple, fixed-sized casts, the compiler will optimise away the memcpy and
the conversion will be free.

> I doubt that the compiler can optimize the memcpy calls away. We profiled a
> release build of d8, and more than 10% of the total runtime are spend in
memcpy.

Neither of these memcpy calls are simple, so the compiler probably can't
optimise them away.

> > another idea in debug would be to compile the simulator itself in release
> mode:
> > there are a fair amount of asserts in the decoder and skipping those should
> > help.

We could do that as an additional build variant, but I'm wary of disabling the
assertions for a full debug build. I'm aware that the A64 port has a lot more
assertions than the ARM port (and probably the others too) so we might be able
to make some compromise, perhaps with another build variant that disables
known-slow assertions at build time.

> The numbers Ulan gave are on a release build.
> 
> > On 2014/02/18 17:35:18, ulan wrote:
> > > PTAL. Is this safe to replace memcpy? This speeds up the simulator by
~20%:
> > > 
> > > Richards: 51.5 (before)
> > > Richards: 63.9 (after)

[jbramley, 2014-02-19 10:12:22]

It might be worth investigating which memcpy calls are problematic, if you
haven't already. I suspect that MemoryRead and MemoryWrite are the main culprits
but that is just a gut feeling.

Also, with a default -O0 debug build, GCC probably doesn't try to optimise
memcpy, so the memcpy calls might have more impact in a debug build than in a
release build. However, there's no reason why a debug build has to be -O0. (I
find that ARM code is easier to read at -O2 anyway, on the rare occasions that I
have to delve into disassembled code.)

If we can't get a type-aliasing-safe implementation (using memcpy) to optimise
properly, another option is to pass -no-strict-aliasing to GCC (at least for the
simulator), which disables all optimisations relating to strict aliasing. Clang
may or may not have an equivalent. It's a bit of a hack though, and it might
disable optimisations elsewhere. (I've no idea what performance impact that
might have. It might be negligible.)

https://codereview.chromium.org/169223004/diff/1/src/a64/instructions-a64.h
File src/a64/instructions-a64.h (left):

https://codereview.chromium.org/169223004/diff/1/src/a64/instructions-a64.h#o...
src/a64/instructions-a64.h:120: memcpy(&bits, this, sizeof(bits));
These simple (fixed-size) memcpy calls should be free. The compiler should
recognise them and compile it as a no-op.

https://codereview.chromium.org/169223004/diff/1/src/a64/simulator-a64.cc
File src/a64/simulator-a64.cc (left):

https://codereview.chromium.org/169223004/diff/1/src/a64/simulator-a64.cc#old...
src/a64/simulator-a64.cc:142: memcpy(stack, &(*it), sizeof(*it));
This copy might not be free because the address needs to be derived from the
iterator. However, this shouldn't be performance-sensitive code.

https://codereview.chromium.org/169223004/diff/1/src/a64/simulator-a64.cc#old...
src/a64/simulator-a64.cc:1472: memcpy(&read, address, num_bytes);
Again, the size is variable so this copy can't be free.

It might be possible for the compiler to optimise it if you split the num_bytes
cases (as you did for reinterpret_cast) but use a fixed memcpy in each case.

https://codereview.chromium.org/169223004/diff/1/src/a64/simulator-a64.cc#old...
src/a64/simulator-a64.cc:1514: memcpy(address, &value, num_bytes);
Ditto.

[Rodolph Perfetta, 2014-02-19 10:12:23]

> > I doubt that the compiler can optimize the memcpy calls away. We profiled a
> > release build of d8, and more than 10% of the total runtime are spend in
> memcpy.

I think I found one of the problems: While MemoryRead and MemoryWrite will call
memcpy since they work on arbitrary length, MemoryRead8/16/32/64 and
MemoryWrite32/64 will not as expected. Our mistake is that when we implemented
the LoadStoreHelper in the simulator instead of using the fixed size
MemoryRead/Write we used the generic version with a variable for size. rewriting
the helper to use the specialised versions (and maybe add MemoryWrite8/16)
should give you some performance back without using reinterpret_cast.

Could you try and see how much do you get this way?

[ulan, 2014-02-19 10:52:40]

Thanks for the comments, Rodolph and Jacob!

I tried fixed width MemoryRead/Write in this CL:
https://codereview.chromium.org/172223002

It improves Richards score to 55.5. The hottest function in perf profile is
Instruction::Bits, memcpy is in top 3.

[jochen (gone - plz use gerrit), 2014-02-19 13:21:53]

On 2014/02/19 10:52:40, ulan wrote:
> Thanks for the comments, Rodolph and Jacob!
> 
> I tried fixed width MemoryRead/Write in this CL:
> https://codereview.chromium.org/172223002
> 
> It improves Richards score to 55.5. The hottest function in perf profile is
> Instruction::Bits, memcpy is in top 3.

I looked at the assembly output, and the memcpy is actually optimized away.

I wanted to point out Instruction::SignedBits already uses the
reinterpret_cast<> magic, so adding it to Bits() at least doesn't make it
worse[tm].

I also tried to force inline Instruction::Bits which makes it disappear from the
perf output, so I guess that's a bit of a red herring anyway. The real problem
is that decoding the instructions takes too long :-/