[AArch64][AsmParser] Remove 'x31' alias for 'sp/xzr' register.

[CarolineConcatto]

Summary

Only the aliases 'xzr' and 'sp' exist for the physical register x31.
The reason for wanting to remove the alias 'x31' is because it allows users
to write invalid asm that is not accepted by the GNU assembler.

Is there any objection to removing this alias? Or do we want to keep
this for compatibility with existing code that uses w31/x31?

[SjoerdMeijer]

If this was allowed:

ldrb	w0, [sp, x31]

but disallowed with this change, then this is a breaking change that doesn't seem reasonable.

The reason for wanting to remove the alias 'x31' is because it allows users to write invalid asm that is not accepted by the GNU assembler.

So, why is this not allowed by the gnu assembler?

[ostannard]

Using x31 is confusing and not compatible with the GNU assembler, which are both god reasons to reject it. Also, the only occurrence of x31 in the architecture manual is this:

There is no register named W31 or X31.

[sdesmalen]

I don't know what route is normally taken in these situations; this change would indeed break code that relies on this 'feature' even though removing parsing support for w31/x31 is correct. An alternative approach would be to emit a warning diagnostic that says the use of w31/x31 is deprecated and that support will be removed in a future release. Is that an approach we want to take, or is it fine to just remove support for it?

[ostannard]

I'm not aware of any official policy about this, but I don't think it's worth going through a multi-release deprecation process for something which is called out in the architecture manual as not valid.

[CarolineConcatto]

• update the release note page on llvm

[sdesmalen]

llvm/docs/ReleaseNotes.rst

98 ↗

(On Diff #301681)

How about simply:

The assembler no longer accepts ``w31`` and ``x31`` as aliases for ``wzr`` and ``xzr``, because the architecture manual explicitly states that no registers with those names exist.

?

[CarolineConcatto]

-update message on the release note for llvm

[sdesmalen]

Thanks for adding the note, LGTM!

[resistor]

I'll be the dissenting voice: I don't think this is the right approach. "It's what gas does" is not a sufficient reason to break user code. I think a warning and *possibly* removing support some time well in the future is the right solution here.

Keep in mind that just because the architecture manual currently says that the name doesn't exist does not mean that it always did. I'm personally aware of a good bit of aarch64 assembly that was written well before the architecture manual was finalized, and there are definitely instances where the specification of the asm syntax was clarified in ways that are incompatible with how it was original stated.

[sdesmalen]

@CarolineConcatto I don't think this patch should have landed because @resistor made some valid arguments on why not to remove support for this directly, but rather to take the approach of emitting a warning instead. Can you please revert the patch?

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

unsigned AArch64AsmParser::matchRegisterNameAlias(StringRef Name,
   if ((RegNum = MatchRegisterName(Name)))
     return Kind == RegKind::Scalar ? RegNum : 0;
 
   if (!RegNum) {
     // Handle a few common aliases of registers.
     if (auto RegNum = StringSwitch<unsigned>(Name.lower())
                     .Case("fp", AArch64::FP)
                     .Case("lr",  AArch64::LR)
-                    .Case("x31", AArch64::XZR)
-                    .Case("w31", AArch64::WZR)
                     .Default(0))
       return Kind == RegKind::Scalar ? RegNum : 0;
 
     // Check for aliases registered via .req. Canonicalize to lower case.
     // That's more consistent since register names are case insensitive, and
     // it's how the original entry was passed in from MC/MCParser/AsmParser.
     auto Entry = RegisterReqs.find(Name.lower());
     if (Entry == RegisterReqs.end())

llvm/test/MC/AArch64/basic-a64-diagnostics.s

 // CHECK-ERROR-AARCH64-NEXT:                   ^
 // CHECK-ERROR-AARCH64-NEXT: error: too few operands for instruction
 // CHECK-ERROR-AARCH64-NEXT:         str q0, [x0, #8]
 // CHECK-ERROR-AARCH64-NEXT:                 ^
 
 //// 32-bit addresses
         ldr w0, [w20]
         ldrsh x3, [wsp]
+        ldrb   w0, [sp, x31]
 // CHECK-ERROR: error: invalid operand for instruction
 // CHECK-ERROR-NEXT:         ldr w0, [w20]
 // CHECK-ERROR-NEXT:                  ^
 // CHECK-ERROR-NEXT: error: invalid operand for instruction
 // CHECK-ERROR-NEXT:         ldrsh x3, [wsp]
 // CHECK-ERROR-NEXT:                    ^
+// CHECK-ERROR-NEXT: error: index must be an integer in range [-256, 255].
+// CHECK-ERROR-NEXT: ldrb   w0, [sp, x31]
+// CHECK-ERROR-NETX:                 ^
 
 //// Store things
         strb w0, [wsp]
         strh w31, [x23, #1]
         str x5, [x22, #12]
         str w7, [x12, #16384]
+        strb   w0, [sp, x31]
 // CHECK-ERROR: error: invalid operand for instruction
 // CHECK-ERROR-NEXT: strb w0, [wsp]
 // CHECK-ERROR-NEXT:           ^
-// CHECK-ERROR-AARCH64: error: invalid operand for instruction
-// CHECK-ERROR-AARCH64-NEXT:         strh w31, [x23, #1]
-// CHECK-ERROR-AARCH64-NEXT:              ^
+// CHECK-ERROR: error: invalid operand for instruction
+// CHECK-ERROR-NEXT:         strh w31, [x23, #1]
+// CHECK-ERROR-NEXT:              ^
 // CHECK-ERROR-AARCH64-NEXT: error: too few operands for instruction
 // CHECK-ERROR-AARCH64-NEXT:         str x5, [x22, #12]
 // CHECK-ERROR-AARCH64-NEXT:                 ^
 // CHECK-ERROR-NEXT: error: {{expected|index must be an}} integer in range [-256, 255]
 // CHECK-ERROR-NEXT:         str w7, [x12, #16384]
 // CHECK-ERROR-NEXT:                 ^
+// CHECK-ERROR-NEXT: error: index must be an integer in range [-256, 255].
+// CHECK-ERROR-NEXT: strb   w0, [sp, x31]
+// CHECK-ERROR-NEXT:                 ^
 
 //// Bad PRFMs
         prfm #-1, [sp]
         prfm #32, [sp, #8]
         prfm pldl1strm, [w3, #8]
         prfm wibble, [sp]
 // CHECK-ERROR-AARCH64: error: Invalid immediate for instruction
 // CHECK-ERROR-ARM64: error: prefetch operand out of range, [0,31] expected

llvm/test/tools/llvm-mca/AArch64/Exynos/load.s

 # NOTE: Assertions have been autogenerated by utils/update_mca_test_checks.py
 # RUN: llvm-mca -march=aarch64 -mcpu=exynos-m3 -resource-pressure=false < %s | FileCheck %s -check-prefixes=ALL,M3
 # RUN: llvm-mca -march=aarch64 -mcpu=exynos-m4 -resource-pressure=false < %s | FileCheck %s -check-prefixes=ALL,M4
 # RUN: llvm-mca -march=aarch64 -mcpu=exynos-m5 -resource-pressure=false < %s | FileCheck %s -check-prefixes=ALL,M5
 
 ldr w0, 1f
 ldur  x0, [sp, #8]
 ldrb  w0, [sp], #1
 ldrsh w0, [sp, #2]!
 ldr x0, [sp, #8]
-ldrb  w0, [sp, x31]
-ldrsh  w0, [sp, x31, lsl #1]
-ldr  w0, [sp, w31, sxtw]
-ldr  x0, [sp, w31, uxtw #3]
+ldrb  w0, [sp, xzr]
+ldrsh  w0, [sp, xzr, lsl #1]
+ldr  w0, [sp, wzr, sxtw]
+ldr  x0, [sp, wzr, uxtw #3]
 ldnp  w0, w1, [sp, #8]
 ldp x0, x1, [sp], #16
 ldpsw  x0, x1, [sp, #8]!
 
 1:
 
 # ALL:      Iterations:        100
 # ALL-NEXT: Instructions:      1200

llvm/test/tools/llvm-mca/AArch64/Exynos/store.s

 # NOTE: Assertions have been autogenerated by utils/update_mca_test_checks.py
 # RUN: llvm-mca -march=aarch64 -mcpu=exynos-m3 -resource-pressure=false -noalias=false < %s | FileCheck %s -check-prefixes=ALL,M3
 # RUN: llvm-mca -march=aarch64 -mcpu=exynos-m4 -resource-pressure=false -noalias=false < %s | FileCheck %s -check-prefixes=ALL,M4
 # RUN: llvm-mca -march=aarch64 -mcpu=exynos-m5 -resource-pressure=false -noalias=false < %s | FileCheck %s -check-prefixes=ALL,M5
 
 stur  x0, [sp, #8]
 strb  w0, [sp], #1
 strh  w0, [sp, #2]!
 str x0, [sp, #8]
-strb  w0, [sp, x31]
-strh  w0, [sp, x31, lsl #1]
-str  w0, [sp, w31, sxtw]
-str  x0, [sp, w31, uxtw #3]
+strb  w0, [sp, xzr]
+strh  w0, [sp, xzr, lsl #1]
+str  w0, [sp, wzr, sxtw]
+str  x0, [sp, wzr, uxtw #3]
 stnp  w0, w1, [sp, #8]
 stp x0, x1, [sp], #16
 stp  w0, w1, [sp, #8]!
 
 # ALL:      Iterations:        100
 # ALL-NEXT: Instructions:      1100
 # ALL-NEXT: Total Cycles:      1303