1071618 - ARM assembler: new instructions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

Attachment: ARMAssembler.diff

ARM assembler extensions to add support for additional instructions:

- SXTB, SXTH, UXTB, UXTH (sign and zero extension)
- LDREX, LDREXH, LDREXB (load-exclusive)
- STREX, STREXH, STREXB (store-exclusive)
- DMB, DSB, ISB (memory barriers for ARMv7)
- DMB_TRAP, DSB_TRAP, ISB_TRAP (memory barriers for ARMv6)
- Predicates and macroinstructions that use those where approprate

Comment 1

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8493785 [details] [diff] [review]
ARMAssembler.diff

Review of attachment 8493785 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  A couple of minor nits.

::: js/src/jit/arm/Assembler-arm.cpp
@@ +1466,5 @@
> +// Sign extension operations.
> +BufferOffset
> +Assembler::as_sxtb(Register dest, Register src, int rotate, Condition c)
> +{
> +    return writeInst(0x06af0070 | (int)c | RD(dest) | ((rotate & 3) << 10) | src.code());

Wow, RM(foo) really should be an alias for foo.code(), rather than the highly specialized useage for mul and div that currently exists.

@@ +1482,5 @@
> +BufferOffset
> +Assembler::as_uxth(Register dest, Register src, int rotate, Condition c)
> +{
> +    return writeInst(0x06ff0070 | (int)c | RD(dest) | ((rotate & 3) << 10) | src.code());
> +}

I would personally prefer to see a named constant 0x06000070, named constants for bits 20-23, and RN(NoAddend), with NoAddend defined to be r15.  A follow-up bug is likely fine for this, but it should be a pretty easy change.

@@ +1871,5 @@
> +
> +BufferOffset
> +Assembler::as_strexb(Register rd, Register rt, Register rn, Condition c)
> +{
> +    return writeInst(0x01c00f90 | (int)c | RD(rd) | RN(rn) | rt.code());

Once again, making a named constant would be nice.

@@ +1879,5 @@
> +
> +BufferOffset
> +Assembler::as_dmb(BarrierOption option)
> +{
> +    return writeInst(0xf57ff050U | (int)option);

This unnamed constant I'm ok with, for some reason.

@@ +1899,5 @@
> +    // See eg https://bugs.kde.org/show_bug.cgi?id=228060.
> +    // ARMv7 manual, "VMSA CP15 c7 register summary".
> +    // Flagged as "legacy" starting with ARMv8, may be disabled on chip, see
> +    // ARMv8 manual E2.7.3 and G3.18.16.
> +    return writeInst(0xee070f9a);

Also ok with this constant.

Comment 2

[Lars T Hansen [:lth]]

Pushed with suggested changes:

https://hg.mozilla.org/integration/mozilla-inbound/rev/67061f79a38a

(Assembler-arm.h appears to already expose a lot more than it really needs to, so I put the new defs in the cpp file, just to make a stand.)

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/67061f79a38a