[AArch64] Asm: Fix parsing of register aliases that have a name starting with 'z'

[sdesmalen]

Summary

This fixes an issue as identified by @rnk in https://reviews.llvm.org/rL321029.

[fhahn]

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

2007

I don't think we need this early exit here. If it's a vector register, matchRegisterNameAlias should return 0 and we return -1 later. We do not bail out for NEON early either.

[fhahn]

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

2007

Ok, I was wrong about matchRegisterNameAlias. But it seems the early exit is not needed, the MC test pass for me without it and we do not have a similar case for NEON.

[sdesmalen]

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

2007

This was added to enable the other test, where the alias is a vector register (that needs custom parsing), e.g.:

peter .req z6
add peter.s, z0.s, z0.s

However, in that case it seems wrong for 'matchRegisterNameAlias(..., RegKind::Scalar) to return the register number of an SVE vector. This is caused by MatchRegisterName() being generic to work for non-scalar registers as well, so I'll fix this.

[sdesmalen]

Fixed issue where matchRegisterNameAlias(..., RegKind::Scalar) also matched non-scalar identifiers.

[rnk]

Thanks!

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

2003

This is unused now.

[fhahn]

LGTM. The problem this solves is that MatchRegisterName parses GPRs, NEON and SVE registers, which leads to surprising behaviour, i.e. matchRegisterNameAlias with RegKind == Scalar would return > 0 for any (valid) Neon & SVE register as well. From the comments and the behaviour, it seems like tryParseRegister should only parse scalar registers, which this patch implements. I think it's worth adding a comment making that clear.

Adding Eric, in case he has some more thoughts!

[fhahn]

One more thing for completeness (there's no need to address this in this patch): currently we try to parse the register before looking up aliases so the following will fail

	z1 .req x6
	add z1, x0, x0
	.unreq z1
// CHECK: add x6, x0, x0

I think GAS tries to apply aliases first, with the following reasoning: Before the SVE registers were added, the snippet above was valid. Now it is not valid any more, as it will parse z1 as a SVE register (the same problem also exists with NEON vector registers)

[sdesmalen]

@rnk Thanks for providing the reduced test-case!
@fhahn Interesting, it seems the assembler had this behaviour even before the SVE patches changed the matchRegisterNameAlias() method.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

return StringSwitch<bool>(Name.lower())
       .Case(".b", true)
       .Case(".h", true)
       .Case(".s", true)
       .Case(".d", true)
       .Case(".q", true)
       .Default(false);
 }
 
-static bool isSVEDataVectorRegister(StringRef Name) {
-  return Name[0] == 'z';
-}
-
 static void parseValidVectorKind(StringRef Name, unsigned &NumElements,
                                  char &ElementKind) {
   assert(isValidVectorKind(Name));
 
   ElementKind = Name.lower()[Name.size() - 1];
   NumElements = 0;
 
   if (Name.size() == 2)
bool AArch64AsmParser::ParseRegister(unsigned &RegNo, SMLoc &StartLoc,
   RegNo = tryParseRegister();
   EndLoc = SMLoc::getFromPointer(getLoc().getPointer() - 1);
   return (RegNo == (unsigned)-1);
 }
 
 // Matches a register name or register alias previously defined by '.req'
 unsigned AArch64AsmParser::matchRegisterNameAlias(StringRef Name,
                                                   RegKind Kind) {
-  unsigned RegNum;
-  switch (Kind) {
-  case RegKind::Scalar:
-    RegNum = MatchRegisterName(Name);
-    break;
-  case RegKind::NeonVector:
-    RegNum = MatchNeonVectorRegName(Name);
-    break;
-  case RegKind::SVEDataVector:
-    RegNum = matchSVEDataVectorRegName(Name);
-    break;
-  }
+  unsigned RegNum = 0;
+  if ((RegNum = matchSVEDataVectorRegName(Name)))
+    return Kind == RegKind::SVEDataVector ? RegNum : 0;
+
+  if ((RegNum = MatchNeonVectorRegName(Name)))
+    return Kind == RegKind::NeonVector ? RegNum : 0;
+
+  // The parsed register must be of RegKind Scalar
+  if ((RegNum = MatchRegisterName(Name)))
+    return Kind == RegKind::Scalar ? RegNum : 0;
 
   if (!RegNum) {
     // 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())
       return 0;

 /// Identifier when called, and if it is a register name the token is eaten and
 /// the register is added to the operand list.
 int AArch64AsmParser::tryParseRegister() {
   MCAsmParser &Parser = getParser();
   const AsmToken &Tok = Parser.getTok();
   if (Tok.isNot(AsmToken::Identifier))
     return -1;
 
   std::string lowerCase = Tok.getString().lower();
-  if (isSVEDataVectorRegister(lowerCase))
-    return -1;
-
   unsigned RegNum = matchRegisterNameAlias(lowerCase, RegKind::Scalar);
+
   // Also handle a few aliases of registers.
   if (RegNum == 0)
     RegNum = StringSwitch<unsigned>(lowerCase)
                  .Case("fp",  AArch64::FP)
                  .Case("lr",  AArch64::LR)
                  .Case("x31", AArch64::XZR)
                  .Case("w31", AArch64::WZR)
                  .Default(0);
 
   if (RegNum == 0)

test/MC/AArch64/SVE/dot-req.s

+// RUN: llvm-mc -triple=aarch64-none-linux-gnu -mattr=+sve -show-encoding < %s 2>&1 | FileCheck %s
+
+foo:
+// CHECK: add z0.s, z1.s, z2.s
+  zbar .req z1
+  add  z0.s, zbar.s, z2.s

test/MC/AArch64/dot-req.s

 // CHECK: fmov    d2, d3                  // encoding: [0x62,0x40,0x60,0x1e]
 // CHECK: ldr      q2, [sp]               // encoding: [0xe2,0x03,0xc0,0x3d]
 // CHECK: mov             v0.8b, v1.8b    // encoding: [0x20,0x1c,0xa1,0x0e]
 
   peter .req x6
   add peter, x0, x0
   .unreq peter
 // CHECK: add x6, x0, x0
+
+  zoe .req x6
+  add zoe, x0, x0
+  .unreq zoe
+// CHECK: add x6, x0, x0