Use movw/movt instead of constant pool on ARMv7

Use movw/movt instead of constant pool on ARMv7.

Some ARM architectures load 32-bit immediate constants more efficiently using movw/movt pairs rather than constant pool loads. This patch allows the assembler to generate one or the other load form at runtime depending on what is faster.

R=ulan@chromium.org

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

[danno, 2012-10-10 12:25:25]

PTAL

[Please use jfb - chromium.org, 2012-10-10 13:56:52]

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm-inl....
src/arm/assembler-arm-inl.h:367: ASSERT(IsMovT(Memory::int32_at(pc + 4)));
+ kInstrSize

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm-inl....
src/arm/assembler-arm-inl.h:369: Instruction* next_instr = Instruction::At(pc +
4);
+ kInstrSize

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm-inl....
src/arm/assembler-arm-inl.h:372: instr->ImmedMovwMovtValue());
The bottom two bits should be zero, which might be an easy JIT-hardening. If the
bottom bit isn't zero and we pass the address to BX or BLX then we're switching
to Thumb, which is a nice exploit vector.

I'd just & ~3

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm-inl....
src/arm/assembler-arm-inl.h:387: // Or pre v8:
Pre v8?

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm-inl....
src/arm/assembler-arm-inl.h:398: IsMovT(Memory::int32_at(candidate + 4)));
+ kInstrSize

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm-inl....
src/arm/assembler-arm-inl.h:443: void Assembler::set_target_address_at(Address
pc, Address target) {
As noted above:
  target &= ~3;

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm-inl....
src/arm/assembler-arm-inl.h:445: ASSERT(IsMovT(Memory::int32_at(pc + 4)));
+ kInstrSize

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm-inl....
src/arm/assembler-arm-inl.h:457: ASSERT(IsMovT(Memory::int32_at(pc + 4)));
+ kInstrSize

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm-inl....
src/arm/assembler-arm-inl.h:467: // accessing this address in the constant pool
remains unchanged.
Is this actually true? I can't find a reference to that effect.

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm.cc#n...
src/arm/assembler-arm.cc:842: s == LeaveCC) {
In this case we're emitting 2 instructions, and when doing GC we need to flush
the I$ for those two instructions. Would it be worth it to prevent MOVW/MOVT
from straddling a cacheline? If at this point it would straddle then we can just
emit a NOP, and then MOVW/MOVT together on the next cacheline. I'm assuming JIT
function's code always starts at a cacheline boundary.

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm.cc#n...
src/arm/assembler-arm.cc:844: RecordRelocInfo(x.rmode_, x.imm32_,
DONT_USE_CONSTANT_POOL);
As before, if there's relocinfo then it's code (right?) so & ~3

http://codereview.chromium.org/11037023/diff/25001/src/arm/macro-assembler-ar...
File src/arm/macro-assembler-arm.cc (right):

http://codereview.chromium.org/11037023/diff/25001/src/arm/macro-assembler-ar...
src/arm/macro-assembler-arm.cc:300: IsPowerOf2(src2.immediate() + 1)) {
It would be kind of silly, but if src2 == 0xffffffff then this is undefined. In
that case it should just be a MVN Rd, 0, which can be handled before this
else_if.

[Michael Starzinger, 2012-10-10 14:19:29]

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm.cc#n...
src/arm/assembler-arm.cc:819: return true;
Indentation is off.

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm.h#ne...
src/arm/assembler-arm.h:696: INLINE(static Address
target_address_from_return_address(Address pc));
Can we have a one-liner comment of what this method does (also on other
architectures).

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm.h#ne...
src/arm/assembler-arm.h:1491: class ScopedPredictableCodeSize {
I would call this PredictableCodeSizeScope instead.

http://codereview.chromium.org/11037023/diff/25001/src/assembler.h
File src/assembler.h (right):

http://codereview.chromium.org/11037023/diff/25001/src/assembler.h#newcode375
src/assembler.h:375: // location provides a place for these pointers to exist
natually
s/natually/naturally/

http://codereview.chromium.org/11037023/diff/25001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (left):

http://codereview.chromium.org/11037023/diff/25001/src/ia32/assembler-ia32.h#...
src/ia32/assembler-ia32.h:1: // Copyright (c) 1994-2006 Sun Microsystems Inc.
Looks like a typo?

http://codereview.chromium.org/11037023/diff/25001/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

http://codereview.chromium.org/11037023/diff/25001/src/ia32/assembler-ia32.h#...
src/ia32/assembler-ia32.h:604: inline static Address
target_address_from_return_address(Address pc);
See comment in ARM assembler.

http://codereview.chromium.org/11037023/diff/25001/src/x64/assembler-x64.h
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/11037023/diff/25001/src/x64/assembler-x64.h#ne...
src/x64/assembler-x64.h:584: static inline Address
target_address_from_return_address(Address pc);
See comment in ARM assembler.

[Michael Starzinger, 2012-10-10 14:34:28]

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/11037023/diff/25001/src/arm/assembler-arm.cc#n...
src/arm/assembler-arm.cc:844: RecordRelocInfo(x.rmode_, x.imm32_,
DONT_USE_CONSTANT_POOL);
On 2012/10/10 13:56:52, jfb wrote:
> As before, if there's relocinfo then it's code (right?) so & ~3

I am not sure about this. We also have RelocInfos for embedded object pointers.
Those will still be aligned to word boundaries, but the general assumption that
relocation infos are just for code targets doesn't hold.

[JF, 2012-10-10 14:47:26]

> I am not sure about this. We also have RelocInfos for embedded object
pointers.
> Those will still be aligned to word boundaries, but the general assumption
that
> relocation infos are just for code targets doesn't hold.

Should the concepts be separated, then?

[Michael Starzinger, 2012-10-10 15:15:50]

On 2012/10/10 14:47:26, JF wrote:
> > I am not sure about this. We also have RelocInfos for embedded object
> pointers.
> > Those will still be aligned to word boundaries, but the general assumption
> that
> > relocation infos are just for code targets doesn't hold.
> 
> Should the concepts be separated, then?

Hmm, I don't think so. We also need relocation infos for embedded objects,
because they also might be moved. The different types are distinguished by the
relocation mode (see RelocInfo::Mode). It might be that V8's RelocInfo is
something different than what other compilers call relocation info. But I think
the current abstraction is totally fine for V8.

[bulach, 2012-10-11 17:23:08]

hey daniel, just to register some of our chats.. :)

https://chromiumcodereview.appspot.com/11037023/diff/20017/src/arm/macro-asse...
File src/arm/macro-assembler-arm.cc (right):

https://chromiumcodereview.appspot.com/11037023/diff/20017/src/arm/macro-asse...
src/arm/macro-assembler-arm.cc:177: //  blx   ip
(just to keep documented)
the minor optimization we thought about was:
if "call address" fits in 24 bits, then do a couple of nop(0) and bl "call
address" directly, which is probably nicer to the pipeline..
there are implications with GC / compaction, but it may be workable.

[danno, 2012-10-17 10:04:44]

Addressed comments and added runtime support for recognizing target platforms
where movw/movt is a win. Please take another look.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
File src/arm/assembler-arm-inl.h (right):

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm-inl.h:367: ASSERT(IsMovT(Memory::int32_at(pc + 4)));
On 2012/10/10 13:56:52, jfb wrote:
> + kInstrSize

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm-inl.h:369: Instruction* next_instr = Instruction::At(pc +
4);
On 2012/10/10 13:56:52, jfb wrote:
> + kInstrSize

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm-inl.h:372: instr->ImmedMovwMovtValue());
On 2012/10/10 13:56:52, jfb wrote:
> The bottom two bits should be zero, which might be an easy JIT-hardening. If
the
> bottom bit isn't zero and we pass the address to BX or BLX then we're
switching
> to Thumb, which is a nice exploit vector.
> 
> I'd just & ~3

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm-inl.h:387: // Or pre v8:
On 2012/10/10 13:56:52, jfb wrote:
> Pre v8?

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm-inl.h:398: IsMovT(Memory::int32_at(candidate + 4)));
On 2012/10/10 13:56:52, jfb wrote:
> + kInstrSize

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm-inl.h:443: void Assembler::set_target_address_at(Address
pc, Address target) {
On 2012/10/10 13:56:52, jfb wrote:
> As noted above:
>   target &= ~3;

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm-inl.h:445: ASSERT(IsMovT(Memory::int32_at(pc + 4)));
On 2012/10/10 13:56:52, jfb wrote:
> + kInstrSize

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm-inl.h:457: ASSERT(IsMovT(Memory::int32_at(pc + 4)));
On 2012/10/10 13:56:52, jfb wrote:
> + kInstrSize

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm-inl.h:467: // accessing this address in the constant pool
remains unchanged.
On 2012/10/10 13:56:52, jfb wrote:
> Is this actually true? I can't find a reference to that effect.

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
File src/arm/assembler-arm.cc (right):

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm.cc:819: return true;
On 2012/10/10 14:19:29, Michael Starzinger wrote:
> Indentation is off.

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm.cc:819: return true;
On 2012/10/10 14:19:29, Michael Starzinger wrote:
> Indentation is off.

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm.cc:842: s == LeaveCC) {
I solved this a little differently. Embedded objects that are likely to be
patched (ICs) don't use the two-instruction sequence.
On 2012/10/10 13:56:52, jfb wrote:
> In this case we're emitting 2 instructions, and when doing GC we need to flush
> the I$ for those two instructions. Would it be worth it to prevent MOVW/MOVT
> from straddling a cacheline? If at this point it would straddle then we can
just
> emit a NOP, and then MOVW/MOVT together on the next cacheline. I'm assuming
JIT
> function's code always starts at a cacheline boundary.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm.cc:844: RecordRelocInfo(x.rmode_, x.imm32_,
DONT_USE_CONSTANT_POOL);
For object pointers, the address is not aligned, so it's not correct to mask
here. I've added masks elsewhere where the code target is known to be aligned. 
On 2012/10/10 14:34:29, Michael Starzinger wrote:
> On 2012/10/10 13:56:52, jfb wrote:
> > As before, if there's relocinfo then it's code (right?) so & ~3
> 
> I am not sure about this. We also have RelocInfos for embedded object
pointers.
> Those will still be aligned to word boundaries, but the general assumption
that
> relocation infos are just for code targets doesn't hold.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
File src/arm/assembler-arm.h (right):

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm.h:696: INLINE(static Address
target_address_from_return_address(Address pc));
On 2012/10/10 14:19:29, Michael Starzinger wrote:
> Can we have a one-liner comment of what this method does (also on other
> architectures).

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/assembler-...
src/arm/assembler-arm.h:1491: class ScopedPredictableCodeSize {
On 2012/10/10 14:19:29, Michael Starzinger wrote:
> I would call this PredictableCodeSizeScope instead.

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/macro-asse...
File src/arm/macro-assembler-arm.cc (right):

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/arm/macro-asse...
src/arm/macro-assembler-arm.cc:300: IsPowerOf2(src2.immediate() + 1)) {
Possible, but that's for a separate CL.
On 2012/10/10 13:56:52, jfb wrote:
> It would be kind of silly, but if src2 == 0xffffffff then this is undefined.
In
> that case it should just be a MVN Rd, 0, which can be handled before this
> else_if.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/assembler.h
File src/assembler.h (right):

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/assembler.h#ne...
src/assembler.h:375: // location provides a place for these pointers to exist
natually
On 2012/10/10 14:19:29, Michael Starzinger wrote:
> s/natually/naturally/

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/ia32/assembler...
File src/ia32/assembler-ia32.h (left):

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/ia32/assembler...
src/ia32/assembler-ia32.h:1: // Copyright (c) 1994-2006 Sun Microsystems Inc.
On 2012/10/10 14:19:29, Michael Starzinger wrote:
> Looks like a typo?

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/ia32/assembler...
File src/ia32/assembler-ia32.h (right):

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/ia32/assembler...
src/ia32/assembler-ia32.h:604: inline static Address
target_address_from_return_address(Address pc);
On 2012/10/10 14:19:29, Michael Starzinger wrote:
> See comment in ARM assembler.

Done.

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/x64/assembler-...
File src/x64/assembler-x64.h (right):

https://chromiumcodereview.appspot.com/11037023/diff/25001/src/x64/assembler-...
src/x64/assembler-x64.h:584: static inline Address
target_address_from_return_address(Address pc);
On 2012/10/10 14:19:29, Michael Starzinger wrote:
> See comment in ARM assembler.

Done.

[JF, 2012-10-17 16:37:58]

Looks good from my limited knowledge of V8.

[ulan, 2012-10-18 09:07:53]

LGTM with comments.

https://chromiumcodereview.appspot.com/11037023/diff/39023/src/arm/assembler-...
File src/arm/assembler-arm.cc (right):

https://chromiumcodereview.appspot.com/11037023/diff/39023/src/arm/assembler-...
src/arm/assembler-arm.cc:843: return true;
Why don't we check for !CpuFeatures::IsSupported(ARMv7) in this case?

https://chromiumcodereview.appspot.com/11037023/diff/39023/src/arm/assembler-...
File src/arm/assembler-arm.h (right):

https://chromiumcodereview.appspot.com/11037023/diff/39023/src/arm/assembler-...
src/arm/assembler-arm.h:757: static const int kPatchDebugBreakSlotReturnOffset =
2 * kInstrSize;
Is the offset correct when we don't use BLX?

https://chromiumcodereview.appspot.com/11037023/diff/39023/src/v8globals.h
File src/v8globals.h (right):

https://chromiumcodereview.appspot.com/11037023/diff/39023/src/v8globals.h#ne...
src/v8globals.h:429: UnknownImplementer,
Nit: names of constants are usually all upper case or prefixed with 'k'.

[danno, 2012-10-18 12:22:10]

Addressed comments, landing.

https://codereview.chromium.org/11037023/diff/39023/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

https://codereview.chromium.org/11037023/diff/39023/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:843: return true;
It's not possible to use immediate loads to the pc to do a call, so loading the
destination address without USE_BLX is always a single instruction of the form
ldr pc, [pc + #xxx].On 2012/10/18 09:07:53, ulan wrote:
> Why don't we check for !CpuFeatures::IsSupported(ARMv7) in this case?

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

https://codereview.chromium.org/11037023/diff/39023/src/arm/assembler-arm.h#n...
src/arm/assembler-arm.h:757: static const int kPatchDebugBreakSlotReturnOffset =
2 * kInstrSize;
On 2012/10/18 09:07:53, ulan wrote:
> Is the offset correct when we don't use BLX?

Done.

https://codereview.chromium.org/11037023/diff/39023/src/v8globals.h
File src/v8globals.h (right):

https://codereview.chromium.org/11037023/diff/39023/src/v8globals.h#newcode429
src/v8globals.h:429: UnknownImplementer,
On 2012/10/18 09:07:53, ulan wrote:
> Nit: names of constants are usually all upper case or prefixed with 'k'.

Done.