[PATCH][AArch64] Restrict MOV alias for ORR instruction

Discussion:

Egeyar Bagcioglu

2018-10-30 18:12:38 UTC

Hello,

ARM Architecture Reference Manual for the profile ARMv8-A, Issue C.a,
states that MOV (register) is an alias of the ORR (shifted register) iff
shift == '00' && imm6 == '000000' && Rn == '11111'. However, mov is
currently preferred for a broader range of orr instructions.

PR 23193 is filed for this purpose and still open. Another related one,
19721 was filed earlier and resolved inconsistently with the manual.

The attached patch fixes the opcodes to match the manual in this regard,
corrects existing test cases and adds new ones. I have re-used the test
source and the driver of the older pr, 19721.

I did *not* replace the existing Rm_SFT used in the "mov" entry with an
Rm. Therefore, "mov" usages with shifted registers such as "mov x7, x17,
lsl 25" could continue to be accepted. However, they are not generated
anymore.

The suggested change log is the following:

2018-10-30Â Egeyar Bagcioglu <***@oracle.com>

Â Â Â [AArch64] Use MOV (register) as an alias for ORR (shifted register)
Â Â Â iff shift == '00' && imm6 == '000000' && Rn == '11111'.

Â Â Â gas/ChangeLog:
Â Â Â Â Â Â Â PR 23193
Â Â Â Â Â Â Â PR 19721
Â Â Â Â Â Â Â * testsuite/gas/aarch64/pr19721.s: Add new test cases.
Â Â Â Â Â Â Â * testsuite/gas/aarch64/pr19721.d: Correct existing test
Â Â Â Â Â Â Â cases and add new ones.

Â Â Â opcodes/ChangeLog:
Â Â Â Â Â Â Â PR 23193
Â Â Â Â Â Â Â PR 19721
Â Â Â Â Â Â Â * aarch64-tbl.h: Use MOV (register) as an alias for ORR (shifted
Â Â Â Â Â Â Â register) iff shift == '00' && imm6 == '000000' && Rn == '11111'.

Tested on aarch64-unknown-linux-gnu and showed no regressions.

Please take a look and apply if legitimate.

Regards,
Egeyar

Richard Earnshaw (lists)

2018-10-31 11:05:55 UTC

Permalink

Thanks for posting this. I'm currently trying to find out whether or
not we want to continue accepting these extended MOV forms. It's
certainly contrary to Arm ARM; but there are some other extensions that
we do deliberately accept for programmer convenience.

R.

Post by Egeyar Bagcioglu
Tested on aarch64-unknown-linux-gnu and showed no regressions.
Please take a look and apply if legitimate.
Regards,
Egeyar
restrict-mov-alias-for-orr.patch
diff --git a/gas/testsuite/gas/aarch64/pr19721.d b/gas/testsuite/gas/aarch64/pr19721.d
index a621ae5..785d2a2 100644
--- a/gas/testsuite/gas/aarch64/pr19721.d
+++ b/gas/testsuite/gas/aarch64/pr19721.d
0: aa1103e7 mov x7, x17
- 4: aa1167e7 mov x7, x17, lsl #25
- 8: aa1167e7 mov x7, x17, lsl #25
+ 4: aa1167e7 orr x7, xzr, x17, lsl #25
+ 8: aa1167e7 orr x7, xzr, x17, lsl #25
+ c: aa4003e0 orr x0, xzr, x0, lsr #0
+ 10: aa0007e0 orr x0, xzr, x0, lsl #1
+ 14: aa0003d0 orr x16, x30, x0
diff --git a/gas/testsuite/gas/aarch64/pr19721.s b/gas/testsuite/gas/aarch64/pr19721.s
index cda068a..be2b508 100644
--- a/gas/testsuite/gas/aarch64/pr19721.s
+++ b/gas/testsuite/gas/aarch64/pr19721.s
@@ -3,3 +3,6 @@
mov x7, x17
mov x7, x17, lsl 25
orr x7, xzr, x17, lsl 25
+ orr x0, xzr, x0, lsr #0 // shift == 01
+ orr x0, xzr, x0, lsl #1 // imm6 == 000001
+ orr x16, x30, x0 // Rn == 11110
diff --git a/opcodes/aarch64-tbl.h b/opcodes/aarch64-tbl.h
index b73007d..df56048 100644
--- a/opcodes/aarch64-tbl.h
+++ b/opcodes/aarch64-tbl.h
@@ -3319,7 +3319,7 @@ struct aarch64_opcode aarch64_opcode_table[] =
CORE_INSN ("and", 0xa000000, 0x7f200000, log_shift, 0, OP3 (Rd, Rn, Rm_SFT), QL_I3SAMER, F_SF),
CORE_INSN ("bic", 0xa200000, 0x7f200000, log_shift, 0, OP3 (Rd, Rn, Rm_SFT), QL_I3SAMER, F_SF),
CORE_INSN ("orr", 0x2a000000, 0x7f200000, log_shift, 0, OP3 (Rd, Rn, Rm_SFT), QL_I3SAMER, F_HAS_ALIAS | F_SF),
- CORE_INSN ("mov", 0x2a0003e0, 0x7f2003e0, log_shift, 0, OP2 (Rd, Rm_SFT), QL_I2SAMER, F_ALIAS | F_SF),
+ CORE_INSN ("mov", 0x2a0003e0, 0x7fe0ffe0, log_shift, 0, OP2 (Rd, Rm_SFT), QL_I2SAMER, F_ALIAS | F_SF),
CORE_INSN ("uxtw", 0x2a0003e0, 0x7f2003e0, log_shift, OP_UXTW, OP2 (Rd, Rm), QL_I2SAMEW, F_ALIAS | F_PSEUDO),
CORE_INSN ("orn", 0x2a200000, 0x7f200000, log_shift, 0, OP3 (Rd, Rn, Rm_SFT), QL_I3SAMER, F_HAS_ALIAS | F_SF),
CORE_INSN ("mvn", 0x2a2003e0, 0x7f2003e0, log_shift, 0, OP2 (Rd, Rm_SFT), QL_I2SAMER, F_ALIAS | F_SF),

Richard Earnshaw (lists)

2018-12-03 17:38:24 UTC

Permalink

After some internal discussions we've agreed that your proposed patch is
probably the best solution here: namely that ORR should be used whenever
the shift is not "lsl #0". So I've committed your patch with some minor
tweaks to the ChangeLog to make it more conformant with the house style.

Thanks for posting this, and sorry for taking so long to get a final answer.

R.

Post by Egeyar Bagcioglu
Hello,
ARM Architecture Reference Manual for the profile ARMv8-A, Issue C.a,
states that MOV (register) is an alias of the ORR (shifted register) iff
shift == '00' && imm6 == '000000' && Rn == '11111'. However, mov is
currently preferred for a broader range of orr instructions.
PR 23193 is filed for this purpose and still open. Another related one,
19721 was filed earlier and resolved inconsistently with the manual.
The attached patch fixes the opcodes to match the manual in this regard,
corrects existing test cases and adds new ones. I have re-used the test
source and the driver of the older pr, 19721.
I did *not* replace the existing Rm_SFT used in the "mov" entry with an
Rm. Therefore, "mov" usages with shifted registers such as "mov x7, x17,
lsl 25" could continue to be accepted. However, they are not generated
anymore.
    [AArch64] Use MOV (register) as an alias for ORR (shifted register)
    iff shift == '00' && imm6 == '000000' && Rn == '11111'.
        PR 23193
        PR 19721
        * testsuite/gas/aarch64/pr19721.s: Add new test cases.
        * testsuite/gas/aarch64/pr19721.d: Correct existing test
        cases and add new ones.
        PR 23193
        PR 19721
        * aarch64-tbl.h: Use MOV (register) as an alias for ORR (shifted
        register) iff shift == '00' && imm6 == '000000' && Rn == '11111'.
Tested on aarch64-unknown-linux-gnu and showed no regressions.
Please take a look and apply if legitimate.
Regards,
Egeyar
restrict-mov-alias-for-orr.patch
diff --git a/gas/testsuite/gas/aarch64/pr19721.d b/gas/testsuite/gas/aarch64/pr19721.d
index a621ae5..785d2a2 100644
--- a/gas/testsuite/gas/aarch64/pr19721.d
+++ b/gas/testsuite/gas/aarch64/pr19721.d
0: aa1103e7 mov x7, x17
- 4: aa1167e7 mov x7, x17, lsl #25
- 8: aa1167e7 mov x7, x17, lsl #25
+ 4: aa1167e7 orr x7, xzr, x17, lsl #25
+ 8: aa1167e7 orr x7, xzr, x17, lsl #25
+ c: aa4003e0 orr x0, xzr, x0, lsr #0
+ 10: aa0007e0 orr x0, xzr, x0, lsl #1
+ 14: aa0003d0 orr x16, x30, x0
diff --git a/gas/testsuite/gas/aarch64/pr19721.s b/gas/testsuite/gas/aarch64/pr19721.s
index cda068a..be2b508 100644
--- a/gas/testsuite/gas/aarch64/pr19721.s
+++ b/gas/testsuite/gas/aarch64/pr19721.s
@@ -3,3 +3,6 @@
mov x7, x17
mov x7, x17, lsl 25
orr x7, xzr, x17, lsl 25
+ orr x0, xzr, x0, lsr #0 // shift == 01
+ orr x0, xzr, x0, lsl #1 // imm6 == 000001
+ orr x16, x30, x0 // Rn == 11110
diff --git a/opcodes/aarch64-tbl.h b/opcodes/aarch64-tbl.h
index b73007d..df56048 100644
--- a/opcodes/aarch64-tbl.h
+++ b/opcodes/aarch64-tbl.h
@@ -3319,7 +3319,7 @@ struct aarch64_opcode aarch64_opcode_table[] =
CORE_INSN ("and", 0xa000000, 0x7f200000, log_shift, 0, OP3 (Rd, Rn, Rm_SFT), QL_I3SAMER, F_SF),
CORE_INSN ("bic", 0xa200000, 0x7f200000, log_shift, 0, OP3 (Rd, Rn, Rm_SFT), QL_I3SAMER, F_SF),
CORE_INSN ("orr", 0x2a000000, 0x7f200000, log_shift, 0, OP3 (Rd, Rn, Rm_SFT), QL_I3SAMER, F_HAS_ALIAS | F_SF),
- CORE_INSN ("mov", 0x2a0003e0, 0x7f2003e0, log_shift, 0, OP2 (Rd, Rm_SFT), QL_I2SAMER, F_ALIAS | F_SF),
+ CORE_INSN ("mov", 0x2a0003e0, 0x7fe0ffe0, log_shift, 0, OP2 (Rd, Rm_SFT), QL_I2SAMER, F_ALIAS | F_SF),
CORE_INSN ("uxtw", 0x2a0003e0, 0x7f2003e0, log_shift, OP_UXTW, OP2 (Rd, Rm), QL_I2SAMEW, F_ALIAS | F_PSEUDO),
CORE_INSN ("orn", 0x2a200000, 0x7f200000, log_shift, 0, OP3 (Rd, Rn, Rm_SFT), QL_I3SAMER, F_HAS_ALIAS | F_SF),
CORE_INSN ("mvn", 0x2a2003e0, 0x7f2003e0, log_shift, 0, OP2 (Rd, Rm_SFT), QL_I2SAMER, F_ALIAS | F_SF),