Avoid flushing the icache unnecessarily when updating target addresses in code.

Avoid flushing the icache unnecessarily when updating target addresses in code.

This CL updates RelocInfo update operations and set_target_address_at to enable
skipping of the icache flush if it going to be batched up later.
Code::CopyFrom and Code::Relocate are modified to avoid individual icache
flushes since the whole code area will be flushed after the reloc info is
updated.

These changes reduce a regression when enabling the OOL constant pool on Arm,
since this change can cause MovT/MovW instructions for relocatable targets
if the constant pool is full.

Scores for Mandreel latency on a Nexus 5:
 - OOL CP disabled: 3533
 - OOL CP enabled, without this CL: 1825
 - OOL CP enabled, with change: 3015

R=rodolph.perfetta@arm.com, ulan@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=21380

[rmcilroy, 2014-05-15 17:35:02]

With this change, the OOL constant pool regressions go away, so I should be able
to finally be able to enable that.  I'll update other architectures if you think
the Arm changes look ok.

Ulan, PTAL, thanks.

[ulan, 2014-05-16 08:38:48]

https://codereview.chromium.org/284153004/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/284153004/diff/1/src/ic.cc#newcode866
src/ic.cc:866: set_target(*pre_monomorphic_stub(), SKIP_ICACHE_FLUSH);
I am not sure whether this is safe. We are leaving icache in an inconsistent
state. Do ARM, ARM64, MIPS guarantee some kind of "atomicity" in this case? I
mean
1) either the old instruction is completely executed
2) or the new instruction is completely executed?

https://codereview.chromium.org/284153004/diff/1/src/ic.cc#newcode1320
src/ic.cc:1320: set_target(*stub, SKIP_ICACHE_FLUSH);
ditto

[rmcilroy, 2014-05-16 13:51:31]

+Rodolph

https://codereview.chromium.org/284153004/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/284153004/diff/1/src/ic.cc#newcode866
src/ic.cc:866: set_target(*pre_monomorphic_stub(), SKIP_ICACHE_FLUSH);
On 2014/05/16 08:38:48, ulan wrote:
> I am not sure whether this is safe. We are leaving icache in an inconsistent
> state. Do ARM, ARM64, MIPS guarantee some kind of "atomicity" in this case? I
> mean
> 1) either the old instruction is completely executed
> 2) or the new instruction is completely executed?

Thus is a good point which I was worried about.  Given that the instructions are
4 byte writes to 4 byte aligned addresses, I think the updates are guaranteed to
be atomic, but I'm not sure (maybe we should use an Atomic32 NoBarrier_Store?). 


I've updated this to ensure that it only does this update if both the
instructions updated are on the same cache line, and then perform a
MemoryBarrier, which I think should ensure the CPU either sees the the whole old
instruction sequence, or the whole new instruction sequence.

Rodolph, is there any guarantees on Arm which we can exploit here, assuming we
don't care whether we execute the old instructions or the new instructions, but
want to ensure we don't execute a mix of old and new?

[Rodolph Perfetta (ARM), 2014-05-16 16:02:28]

https://codereview.chromium.org/284153004/diff/20001/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/284153004/diff/20001/src/arm/assembler-arm-in...
src/arm/assembler-arm-inl.h:543: MemoryBarrier();
Instruction prefetching is implementation defined so what you are trying to
achieve is not guaranteed to work, as being on the same cache line does not
guarantee being fetched atomically (though on most CPU it probably does).

You could make it work if there was only one instruction, but with two I don't
see a way to avoid cache maintenance. Sorry.

As a separate note, MemoryBarrier will guarantee the write above it will happen
before the write after but it does not guarantee they will be visible to the
instruction side of the CPU.

[rmcilroy, 2014-05-16 16:15:43]

https://codereview.chromium.org/284153004/diff/20001/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/284153004/diff/20001/src/arm/assembler-arm-in...
src/arm/assembler-arm-inl.h:543: MemoryBarrier();
On 2014/05/16 16:02:29, Rodolph Perfetta (ARM) wrote:
> Instruction prefetching is implementation defined so what you are trying to
> achieve is not guaranteed to work, as being on the same cache line does not
> guarantee being fetched atomically (though on most CPU it probably does).
> 
> You could make it work if there was only one instruction, but with two I don't
> see a way to avoid cache maintenance. Sorry.

How about if we only need to update one of the instructions then? It is likely
that the MovT probably doesn't need to be updated for most IC cache updates, so
we can maybe get away with this if only the MovW needs updated - is there
anything else I would need to do if I was only updating a single instruction?

> As a separate note, MemoryBarrier will guarantee the write above it will
happen
> before the write after but it does not guarantee they will be visible to the
> instruction side of the CPU.

Yes, I was unsure whether the kernel defined barrier function would do a dmb and
imb, or just a dmb - sounds like it's just a dmb?  If so then this is indeed
useless.

[Rodolph Perfetta (ARM), 2014-05-16 16:31:56]

https://codereview.chromium.org/284153004/diff/20001/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/284153004/diff/20001/src/arm/assembler-arm-in...
src/arm/assembler-arm-inl.h:543: MemoryBarrier();
> How about if we only need to update one of the instructions then? It is likely
> that the MovT probably doesn't need to be updated for most IC cache updates,
so
> we can maybe get away with this if only the MovW needs updated - is there
> anything else I would need to do if I was only updating a single instruction?

No nothing else, that should work. Though if one day V8 were to do concurrent
patching then it won't work anymore.

[rmcilroy, 2014-05-19 11:00:26]

On 2014/05/16 16:31:56, Rodolph Perfetta (ARM) wrote:
>
https://codereview.chromium.org/284153004/diff/20001/src/arm/assembler-arm-inl.h
> File src/arm/assembler-arm-inl.h (right):
> 
>
https://codereview.chromium.org/284153004/diff/20001/src/arm/assembler-arm-in...
> src/arm/assembler-arm-inl.h:543: MemoryBarrier();
> > How about if we only need to update one of the instructions then? It is
likely
> > that the MovT probably doesn't need to be updated for most IC cache updates,
> so
> > we can maybe get away with this if only the MovW needs updated - is there
> > anything else I would need to do if I was only updating a single
instruction?
> 
> No nothing else, that should work. Though if one day V8 were to do concurrent
> patching then it won't work anymore.

Updated to only skip if a single instruction as discussed.  PTAL.

[ulan, 2014-05-19 11:42:51]

Ross, how much does "skip if atomic" improve the benchmark?

> OOL CP enabled, with change to Code::CopyFrom: 2827 
Maybe we can land with only the Code::CopyFrom change but without "skip if
atomic"?

[Rodolph Perfetta, 2014-05-19 12:35:20]

On 2014/05/19 11:42:51, ulan wrote:
> Ross, how much does "skip if atomic" improve the benchmark?
> 
> > OOL CP enabled, with change to Code::CopyFrom: 2827 
> Maybe we can land with only the Code::CopyFrom change but without "skip if
> atomic"?

If you only apply this change to CopyFrom then maybe a scope would make things
simpler: outside the scope flush, inside skip, no update is required to the
function definitions.

[Rodolph Perfetta, 2014-05-19 12:38:38]

https://codereview.chromium.org/284153004/diff/40001/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/284153004/diff/40001/src/arm/assembler-arm-in...
src/arm/assembler-arm-inl.h:513: static uint32_t ExtractMovwImmediate(Instr
instruction) {
the Instruction class has a ImmedMovwMovtValue method which will do this.

[rmcilroy, 2014-05-19 16:52:15]

Updated to remove skipping during IC updates as discussed offline.  Also added
other architectures and updated the CL description.  PTAL.

> If you only apply this change to CopyFrom then maybe a scope would make things
> simpler: outside the scope flush, inside skip, no update is required to the
> function definitions.

I thought of this, however it's not easy to do since set_target_address_at is
static, so there is nothing which we can figure out whether we are inside or
outside the scope except for an argument anyway.

https://codereview.chromium.org/284153004/diff/40001/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/284153004/diff/40001/src/arm/assembler-arm-in...
src/arm/assembler-arm-inl.h:513: static uint32_t ExtractMovwImmediate(Instr
instruction) {
On 2014/05/19 12:38:38, Rodolph Perfetta wrote:
> the Instruction class has a ImmedMovwMovtValue method which will do this.

Ack (no longer applicable).

[ulan, 2014-05-20 08:16:33]

Thank you, lgtm.

https://codereview.chromium.org/284153004/diff/60001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

https://codereview.chromium.org/284153004/diff/60001/src/arm/assembler-arm.h#...
src/arm/assembler-arm.h:768: FLUSH_ICACHE_IF_NEEDED));
Nit: 4 space identation here and in other assembler-*.h files

https://codereview.chromium.org/284153004/diff/60001/src/arm/assembler-arm.h#...
src/arm/assembler-arm.h:777: FLUSH_ICACHE_IF_NEEDED)) {
ditto.

[Rodolph Perfetta (ARM), 2014-05-20 08:51:10]

lgtm

[rmcilroy, 2014-05-20 09:21:03]

https://codereview.chromium.org/284153004/diff/60001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

https://codereview.chromium.org/284153004/diff/60001/src/arm/assembler-arm.h#...
src/arm/assembler-arm.h:768: FLUSH_ICACHE_IF_NEEDED));
On 2014/05/20 08:16:34, ulan wrote:
> Nit: 4 space identation here and in other assembler-*.h files

Grrr - editor decided that two tab's should equal 3 spaces for some reason...
Done.

https://codereview.chromium.org/284153004/diff/60001/src/arm/assembler-arm.h#...
src/arm/assembler-arm.h:777: FLUSH_ICACHE_IF_NEEDED)) {
On 2014/05/20 08:16:34, ulan wrote:
> ditto.

Done.

[rmcilroy, 2014-05-20 09:22:05]

Committed patchset #5 manually as r21380 (presubmit successful).