ARM backend support for pld (preload data) instruction

ARM backend support for pld (preload data) instruction
BUG=none
TEST=none

[rkrithiv, 2013-02-27 22:29:36]

Hi Danno,
I would appreciate it if you could review my change.
Thanks!

[danno, 2013-02-28 07:06:32]

Thanks for the patch. Just some nits, please address these and then I can land
this for you.

https://codereview.chromium.org/12367002/diff/1/src/arm/constants-arm.h
File src/arm/constants-arm.h (right):

https://codereview.chromium.org/12367002/diff/1/src/arm/constants-arm.h#newco...
src/arm/constants-arm.h:712: inline bool IsPld() const { return (Bits(31, 28) ==
15)
nit: formatting, please don't put code on the line after { unless it fits on a
single line. Also ensure arguments line up on subsequent lines according to
style guide.

https://codereview.chromium.org/12367002/diff/1/src/arm/constants-arm.h#newco...
src/arm/constants-arm.h:716: && (Bits(15, 12) == 15); }
} on next line by itself.

[Sven Panne, 2013-02-28 07:36:11]

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

https://codereview.chromium.org/12367002/diff/1/src/arm/assembler-arm.cc#newc...
src/arm/assembler-arm.cc:1499: void Assembler::pld(const MemOperand& src) {
This looks suspiciously like a specialization of addrmod2. Can't we reuse
addrmod2 here to avoid duplicating more than 20 lines?

https://codereview.chromium.org/12367002/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/12367002/diff/1/src/arm/simulator-arm.cc#newc...
src/arm/simulator-arm.cc:3251: if (!instr->IsPld()) {
Hmmm, shouldn't we at least do the load here in the case of pld? If e.g. the
address is nonsense, we would see the effect here.

[danno, 2013-02-28 07:53:55]

https://codereview.chromium.org/12367002/diff/1/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/12367002/diff/1/src/arm/simulator-arm.cc#newc...
src/arm/simulator-arm.cc:3251: if (!instr->IsPld()) {
I'm not sure that's the right thing to do.

From ARM ARM:

"The Preload instructions are hints, and so implementations can treat them as
NOPs without affecting the functional behavior of the device."

This suggests that bad addresses are simply ignored and do not cause exceptions,
so dereffing the address and possibly crashing doesn't seem spec-conform.

Adding some other ARM-experts

On 2013/02/28 07:36:11, Sven Panne wrote:
> Hmmm, shouldn't we at least do the load here in the case of pld? If e.g. the
> address is nonsense, we would see the effect here.

[danno, 2013-04-04 13:10:56]

Are you still interested in landing this patch? If so, please take a look and
respond to my comments. Thanks!

[rkrithiv, 2013-04-05 00:16:53]

I have fixed the formatting of IsPld() in patch set #2.
As for the similarities with addrmod2, there does not seem to be any restriction
on Rn being pc in the pld instruction (this is checked in the final ASSERT in
addrmod2), so I decided not to use addrmod2 for pld.

https://codereview.chromium.org/12367002/diff/1/src/arm/constants-arm.h
File src/arm/constants-arm.h (right):

https://codereview.chromium.org/12367002/diff/1/src/arm/constants-arm.h#newco...
src/arm/constants-arm.h:716: && (Bits(15, 12) == 15); }
On 2013/02/28 07:06:32, danno wrote:
> } on next line by itself.

Done.

[ulan, 2013-04-08 12:28:56]

> As for the similarities with addrmod2, there does not seem to be any
restriction
> on Rn being pc in the pld instruction (this is checked in the final ASSERT in
> addrmod2), so I decided not to use addrmod2 for pld.

The assert in addrmod2 is

ASSERT((am & (P|W)) == P || !x.rn_.is(pc));

The first part (am & (P|W)) == P holds for pld, so Rn can be pc.

I also think it is better to use addrmod2, but special check should be added to
ensure that offset_12 fits into 12 bits.