Use VLDR instead of VMOVs from GPR when a 64-bit double can't be encoded as a VMOV immediate.

Use VLDR instead of VMOVs from GPR when a 64-bit double can't be encoded as a VMOV immediate.

This requires constant blinding before it can be enabled. There are other interesting optimizations that can be added later, detailed in a TODO.

BUG=optimization
R=ulan@chromium.org,mstarzinger@chromium.org, hwennborg@google.com

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

[ulan, 2012-10-18 13:36:53]

I didn't look in depth yet, just one suggestion: it might worthwhile to extract
the Operand::Zero() cleanup into a separate CL as it doesn't depend on VMOV.

https://chromiumcodereview.appspot.com/11191029/diff/1/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (left):

https://chromiumcodereview.appspot.com/11191029/diff/1/src/arm/assembler-arm....
src/arm/assembler-arm.cc:53: 
This line seems to be accidentally deleted.

[JF, 2012-10-19 02:19:44]

Ah yes, sorry about that: I renamed RelocInfo::NONE32 and changed more than I
though I would. I reverted that part and will do it separately.

I've re-uploaded a smaller changelist, PTAL.

[ulan, 2012-10-22 09:18:25]

Looks good overall, but I would like Michael to take a look too since I don't
know reloc infos very well.

https://chromiumcodereview.appspot.com/11191029/diff/4001/src/arm/assembler-a...
File src/arm/assembler-arm.cc (right):

https://chromiumcodereview.appspot.com/11191029/diff/4001/src/arm/assembler-a...
src/arm/assembler-arm.cc:462: 
Functions should be separated by two empty lines.

https://chromiumcodereview.appspot.com/11191029/diff/4001/src/arm/assembler-a...
src/arm/assembler-arm.cc:2664: (pc_limit - first_const_pool_use_ <
kMaxDistToIntPool));
Maybe also add assert for kMaxDistToFPPool if there are pending 64 bit entries.

https://chromiumcodereview.appspot.com/11191029/diff/4001/src/arm/assembler-a...
src/arm/assembler-arm.cc:2698: bool has_fp_values =
(num_pending_64_bit_reloc_info_ > 0);
Indentation.

https://chromiumcodereview.appspot.com/11191029/diff/4001/src/arm/assembler-a...
src/arm/assembler-arm.cc:2705: size += num_pending_64_bit_reloc_info_ *
(kDoubleSize / 2);
Why is it (kDoubleSize / 2)?

https://chromiumcodereview.appspot.com/11191029/diff/4001/src/arm/assembler-a...
src/arm/assembler-arm.cc:2758: ASSERT(!((uintptr_t)pc_ & 0x3));  // Check 64-bit
alignment.
Shouldn't this assert be after we check for NONE64?

https://chromiumcodereview.appspot.com/11191029/diff/4001/src/assembler.h
File src/assembler.h (right):

https://chromiumcodereview.appspot.com/11191029/diff/4001/src/assembler.h#new...
src/assembler.h:225: explicit RelocInfo(byte* pc, Mode rmode, intptr_t data,
Code* host)
No need for explicit here and below since the number of arguments != 1

[ulan, 2012-10-22 14:33:21]

[+v8-dev]

[JF, 2012-10-22 17:11:46]

>
https://chromiumcodereview.appspot.com/11191029/diff/4001/src/arm/assembler-a...
> src/arm/assembler-arm.cc:462: 
> Functions should be separated by two empty lines.

Done.

>
https://chromiumcodereview.appspot.com/11191029/diff/4001/src/arm/assembler-a...
> src/arm/assembler-arm.cc:2664: (pc_limit - first_const_pool_use_ <
> kMaxDistToIntPool));
> Maybe also add assert for kMaxDistToFPPool if there are pending 64 bit
entries.

For that I'd need to split up the 32- and 64-bit pools, and keep track of where
the first 64-bit entry appears. I want to do this separately from the current
changes, but I'll add a TODO at that line.

>
https://chromiumcodereview.appspot.com/11191029/diff/4001/src/arm/assembler-a...
> src/arm/assembler-arm.cc:2698: bool has_fp_values =
> (num_pending_64_bit_reloc_info_ > 0);
> Indentation.

Done.

>
https://chromiumcodereview.appspot.com/11191029/diff/4001/src/arm/assembler-a...
> src/arm/assembler-arm.cc:2705: size += num_pending_64_bit_reloc_info_ *
> (kDoubleSize / 2);
> Why is it (kDoubleSize / 2)?

Added a comment. Splitting the 64-bit entries as explained above will simplify
this code.

>
https://chromiumcodereview.appspot.com/11191029/diff/4001/src/arm/assembler-a...
> src/arm/assembler-arm.cc:2758: ASSERT(!((uintptr_t)pc_ & 0x3));  // Check
64-bit
> alignment.
> Shouldn't this assert be after we check for NONE64?

Done.

>
https://chromiumcodereview.appspot.com/11191029/diff/4001/src/assembler.h#new...
> src/assembler.h:225: explicit RelocInfo(byte* pc, Mode rmode, intptr_t data,
> Code* host)
> No need for explicit here and below since the number of arguments != 1

Done.

[Michael Starzinger, 2012-10-25 13:03:37]

LGTM (with a macro-nit).

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

https://codereview.chromium.org/11191029/diff/11001/src/arm/assembler-arm.cc#...
src/arm/assembler-arm.cc:2806: GetLdrRegisterImmediateOffset(instr) == 0));
This change seems obsolete.

[Michael Starzinger, 2012-10-25 13:20:08]

So I am not sure if you want to landed right away or if you want to address the
constant blinding TODO first. Just let me know and I land it for you.

[JF, 2012-10-25 14:59:40]

On 2012/10/25 13:20:08, Michael Starzinger wrote:
> So I am not sure if you want to landed right away or if you want to address
the
> constant blinding TODO first. Just let me know and I land it for you.

I definitely don't want to ship this without constant blinding, but I'd like to
prevent it from diverging from the rest of the code base. Can I check it in
turned off, and submit the constant blinding in the next few days (and how)?

[Michael Starzinger, 2012-10-25 15:11:33]

You could add a flag for that in flag-definitions.h, default that flag to false
and then check for it in Assembler::vmov() and emit the old code if the flag
wasn't passed. We already have a few "ARM only" flags in there.

[JF, 2012-10-25 15:46:33]

On 2012/10/25 15:11:33, Michael Starzinger wrote:
> You could add a flag for that in flag-definitions.h, default that flag to
false
> and then check for it in Assembler::vmov() and emit the old code if the flag
> wasn't passed. We already have a few "ARM only" flags in there.

Sounds good, I'll fix your comments and add that flag.

[hans, 2012-11-26 11:59:10]

Any update on this?

I'm looking forward to seeing this patch landed, because the "synthesise the
double from ARM immediates" part in Assembler::vmov becomes problematic when
enabling use of D16-D31, which do not have any corresponding S-registers.

[JF, 2012-11-26 16:52:54]

Sorry for the delay, I've been trying to get something in for NaCl before our
next ship date and haven't come back to this yet. I think I'll be able to check
it in (with the flag Michael suggested) within the next week.

It still won't be a solution for everything, though: it really needs constant
blinding or something equivalent before it can be used.

[JF, 2012-11-30 21:43:04]

I've finally come back to this issue and put it behind a flag that defaults to
false until we get proper constant blinding.

PTAL.

[ulan, 2012-12-04 10:13:37]

Thanks, JF! It looks good with one comment.

Do we land it behind the flag and then land constant blinding? I am a bit
worried about constant blinding performance. Could it happen it introduces perf
regressions that would prevent landing it?

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

https://chromiumcodereview.appspot.com/11191029/diff/20001/src/arm/assembler-...
src/arm/assembler-arm.cc:2875: ASSERT(rinfo.rmode() == RelocInfo::NONE64);
We already emitted 64-bit entries, so this code should be unreachable.

[JF, 2012-12-04 17:38:17]

> Do we land it behind the flag and then land constant blinding? I am a bit
> worried about constant blinding performance. Could it happen it introduces
perf
> regressions that would prevent landing it?

I think we need constant blinding or something similar, otherwise an attacker
could be clever about doubles that they get us to emit in the constant pool.
It's currently very easy to know the order they'll get emitted in, and the page
is marked as executable, so it would be too easy for an attacker to have
arbitrary code creation, all they need after is one arbitrary code execution to
own us.

Constant blinding is one way to work around the issue, but we could also
randomize the order (which might have a bad impact on locality), or do like NaCl
and add BKPT instructions in the middle of constant pools.

I think it'll be possible to enable this without performance regressions :-)

We should discuss the best way to make this safe with security people, I'll
start a separate thread.

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

https://chromiumcodereview.appspot.com/11191029/diff/20001/src/arm/assembler-...
src/arm/assembler-arm.cc:2875: ASSERT(rinfo.rmode() == RelocInfo::NONE64);
On 2012/12/04 10:13:37, ulan wrote:
> We already emitted 64-bit entries, so this code should be unreachable.

Ah yes, I think the code didn't merge well, I remember fixing this previously
when I split up 64- and 32-bit code emitting.

I'll remove it.

[JF, 2012-12-06 18:46:40]

DO you want to check in the patch for now, to prevent it from being hard to
merge later? I'm pretty sure it'll be possible to use the feature at least in a
restricted way to get a performance increase, but right now it seems like nobody
has time to properly discuss how to do this in an acceptable way.

[hans, 2012-12-06 18:52:26]

On 2012/12/06 18:46:40, JF wrote:
> DO you want to check in the patch for now, to prevent it from being hard to
> merge later? I'm pretty sure it'll be possible to use the feature at least in
a
> restricted way to get a performance increase, but right now it seems like
nobody
> has time to properly discuss how to do this in an acceptable way.

FWIW, I'm not blocked on this for the d16-d31 patch since Rodolph pointed out we
can use "vmov.32 Dd[x], Rt" to get immediates into a d-register without
corresponding s-registers.

[ulan, 2012-12-10 10:28:30]

> DO you want to check in the patch for now, to prevent it from being hard to
Since we are not going to enable this for M25 anyway, let's land it next week
after we make a branch for M25.

On 2012/12/06 18:52:26, hans wrote:
> On 2012/12/06 18:46:40, JF wrote:
> > DO you want to check in the patch for now, to prevent it from being hard to
> > merge later? I'm pretty sure it'll be possible to use the feature at least
in
> a
> > restricted way to get a performance increase, but right now it seems like
> nobody
> > has time to properly discuss how to do this in an acceptable way.
> 
> FWIW, I'm not blocked on this for the d16-d31 patch since Rodolph pointed out
we
> can use "vmov.32 Dd[x], Rt" to get immediates into a d-register without
> corresponding s-registers.

[JF, 2012-12-10 16:55:38]

On 2012/12/10 10:28:30, ulan wrote:
> > DO you want to check in the patch for now, to prevent it from being hard to
> Since we are not going to enable this for M25 anyway, let's land it next week
> after we make a branch for M25.

Sounds good.

[ulan, 2012-12-28 13:12:48]

LGTM, I will fix the nit and land it for you.

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

https://chromiumcodereview.appspot.com/11191029/diff/20002/src/arm/assembler-...
src/arm/assembler-arm.cc:2861: ASSERT(!IsVldrDPcImmediateOffset(instr));
I'll move this after the instr initialization, otherwise it doesn't compile in
debug mode.