Improve page flag checking sequence on ARM.

Improve page flag checking sequence on ARM.

R=ulan@chromium.org

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

[Please use jfb - chromium.org, 2012-10-09 12:59:58]

LGTM, pending perf results.

[Please use jfb - chromium.org, 2012-10-09 13:09:21]

https://codereview.chromium.org/11090021/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/11090021/diff/1/src/arm/macro-assembler-arm.c...
src/arm/macro-assembler-arm.cc:3500: Move(scratch, object);
Actually, let me retract my LGTM: this will generate an extra move on non-ARMv7
machines. The move should be emitted with the same condition as Bfc:
if (!CpuFeatures::IsSupported(ARMv7) || predictable_code_size())

[m.m.capewell, 2012-10-09 13:25:27]

https://codereview.chromium.org/11090021/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/11090021/diff/1/src/arm/macro-assembler-arm.c...
src/arm/macro-assembler-arm.cc:3501: Bfc(scratch, 0, kPageSizeBits);
A more generic way of doing this, possibly with slightly better performance, is
using two shifts, right then left, by kPageSizeBits.

[Michael Starzinger, 2012-10-09 13:36:19]

https://codereview.chromium.org/11090021/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/11090021/diff/1/src/arm/macro-assembler-arm.c...
src/arm/macro-assembler-arm.cc:3500: Move(scratch, object);
On 2012/10/09 13:09:21, jfb wrote:
> Actually, let me retract my LGTM: this will generate an extra move on
non-ARMv7
> machines. The move should be emitted with the same condition as Bfc:
> if (!CpuFeatures::IsSupported(ARMv7) || predictable_code_size())

Done. You are right, I totally missed that.

[ulan, 2012-10-09 13:41:29]

LGTM

[Michael Starzinger, 2012-10-09 13:54:34]

https://codereview.chromium.org/11090021/diff/1/src/arm/macro-assembler-arm.cc
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/11090021/diff/1/src/arm/macro-assembler-arm.c...
src/arm/macro-assembler-arm.cc:3501: Bfc(scratch, 0, kPageSizeBits);
On 2012/10/09 13:25:27, m.m.capewell wrote:
> A more generic way of doing this, possibly with slightly better performance,
is
> using two shifts, right then left, by kPageSizeBits.

So just to clarify. Do you think that the two-shift-approach would perform
better than move+bfc on ARMv7? That would surprise me, but I am no ARM expert.

My approach would be to always use move+bfc on ARMv7 and only use the
two-shift-approach when we are running on non-ARMv7 and (lsb == 0). Need to find
a non-ARMv7 device for measurements.

[m.m.capewell, 2012-10-09 14:07:27]

> So just to clarify. Do you think that the two-shift-approach would perform
> better than move+bfc on ARMv7? That would surprise me, but I am no ARM expert.

If you look at the A9 Technical Reference Manual, this page explains that BFC is
a two cycle instructions, whereas MOV with constant shift is single cycle:
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0388i/Chdgjcci.html

So shift right/left will be two cycles, and MOV + BFC three. However, if the
source and destination register are the same, only BFC would be required, and
would take two cycles with slightly smaller code.

The only way to be sure is to benchmark it ;)