Improve page flag checking sequence on ARM. (issue 11090021)

[mstar...@chromium.org]

Reviewers: ulan, JF,

Description:
Improve page flag checking sequence on ARM.

R=ul...@chromium.org


Please review this at https://codereview.chromium.org/11090021/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/arm/macro-assembler-arm.cc


Index: src/arm/macro-assembler-arm.cc
diff --git a/src/arm/macro-assembler-arm.cc b/src/arm/macro-assembler-arm.cc
index
cc7fc4d3a4203706ce0a9cccee63e43d2adbdbb2..cf95e77f0771c346e28c31943a50f0861dae587b
100644
--- a/src/arm/macro-assembler-arm.cc
+++ b/src/arm/macro-assembler-arm.cc
@@ -3497,7 +3497,8 @@ void MacroAssembler::CheckPageFlag(
int mask,
Condition cc,
Label* condition_met) {
- and_(scratch, object, Operand(~Page::kPageAlignmentMask));
+ Move(scratch, object);
+ Bfc(scratch, 0, kPageSizeBits);
ldr(scratch, MemOperand(scratch, MemoryChunk::kFlagsOffset));
tst(scratch, Operand(mask));
b(cc, condition_met);

[j...@google.com]

LGTM, pending perf results.

https://codereview.chromium.org/11090021/

[j...@google.com]

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.cc#newcode3500
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())

https://codereview.chromium.org/11090021/

[m.m.ca...@googlemail.com]

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.cc#newcode3501
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.

https://codereview.chromium.org/11090021/

[mstar...@chromium.org]

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.cc#newcode3500
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.

https://codereview.chromium.org/11090021/

[ul...@chromium.org]

LGTM

https://codereview.chromium.org/11090021/

[mstar...@chromium.org]

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.cc#newcode3501
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.

https://codereview.chromium.org/11090021/

[m.m.ca...@googlemail.com]

> 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 ;)

https://codereview.chromium.org/11090021/