[AArch64][SVE] Asm: Replace 'IsVector' by 'RegKind' in AArch64AsmParser (NFC)

[sdesmalen]

Summary

Patch [2/5] in a series to add assembler/disassembler support for AArch64 SVE unpredicated ADD/SUB instructions.

This change is a non functional change that adds RegKind as an alternative to 'isVector' to prepare it for newer types (SVE data vectors and predicate vectors) that will be added in next patches (where the SVE data vector is added as part of this patch set)

[fhahn]

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

62

Would it make sense to use something more generic for the names here, like ScalableVector?

811

Personally I think having predicate functions like Reg.isScalar(), Reg.isNeonVector() would be slightly more compact, but I think either way is fine.

812

From looking at the functions around here, it seems like a space would be good here.

1889

What is the reason for moving this code out to a separate function? Is it to not change the interface of matchRegisterNameAlias, which still takes bool isVector as an argument?

1905

I think keeping the boolean isVector argument has potential to confuse people. Any reason for not being explicit here and pass RegKind here? Because there are 2 vector register types now, isVector seems ambiguous.

1911

space between matchRegisterNameAlias and ()?

2589

Any reason for not passing RegKind::NeonVector directly here? It seems like RegisterKind is not used anywhere else around here.

2590

stray new line?

2771

unrelated change?

[javed.absar]

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

62

It seems to be it would be more maintainable to split it

• RegKind { Scalar, Vector }; and then

enum class VectorKind { NeonVector, SVEVector};

This might simplify the checking logic further down.

[sdesmalen]

Refactored code to reflect some of the suggestions.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

62

@fhahn A subsequent patch will also add a SVE Predicate Vector.

@javed.absar I had a look at splitting it into RegKind { Scalar, Vector }; and subsequently into VectorKind {NeonVector, SVEVector}, but I don't easily see how this makes the checking logic any simpler. Did you have a specific check that you think would be simplified by this?

811

I think that is basically what we're implementing here, but hen for 'isReg()'. See for example 'isVectorReg()', which is the opposite of 'isScalar()'. I see now that isVectorReg() is only checking for a NeonVector, which seems inconsistent with its name. I'll see if I can fix that.

1889

We initially moved it into a separate function to reuse it for 'matchSVEVectorNameAlias()' (as added in patch 3/5)... But, looking at it again I see it makes more sense to add to the same function and use RegKind for each (which eliminates the purpose of moving this into a separate function). Please check if you're happy with the next iteration of this patch.

1905

should be addressed by the next iteration of this patch (see previous comment)

[rengolin]

So, overall, this is a sensible change. We must be able to separate Neon vectors from SVE vectors. I'm not sure how the predicates will fare here and why they wouldn't be SVE vectors (surely, they're scalable, too).

I would prefer to keep isVector and add isScalable, and move the distinction between Neon and SVE to helper functions. But I'm not holding this opinion firmly, as I haven't seen the rest of the code.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

62

Why won't SVE predicate not be an "SVEVector"?

814

Is there a situation (in the future) that we might want?

bool isVector() const { return Kind == k_Register && !Reg.isScalar; }

1910

not that it matters much but the return change here is pointless. :)

1914

function naming consistency.

2771

just checked, this is not violating 80-columns. :)

[sdesmalen]

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

62

Fair point, it should probably distinguish Data and Predicate in the name. I'll change this.
(also, since this is not yet used until the next patch in this series, I will move this change to that patch)

814

At the moment I haven't found such a situation yet (even when considering other SVE assembler/disassembler patches) and I would rather not add code that isn't used anywhere.
So I suggest adding that only when needed.

[rengolin]

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

814

Makes sense

[rengolin]

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

62

nit: space alignment.

[rengolin]

Adding a few AArch64 folks, to make sure we get enough eyes on this, thus avoiding silly problems in the future, if we miss anything.

[sdesmalen]

Removed whitespace.

[fhahn]

A few minor nits, but this looks good to me now. @rengolin what do you think?

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

1910

nit: unrelated whitespace change

1911

nit: unrelated whitespace change.

2589

Do we need the RegisterKind variable? Doesn't look like it is used somewhere else.

[sdesmalen]

• Propagated 'RegKind::NeonVector' for variable RegisterKind with single use.
• Removed newline.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

1910

I don't feel strongly about it, but I think it improves readability.

2589

I thought I had addressed it in my previous patch, but I must have missed it. Please see updated patch.

[rengolin]

Small nit, but overall, looks good to me.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

2625

nit: shouldn't this function be called tryParseNeonVectorRegister, then?

[fhahn]

Thanks Sander, looks good to me too, with the nit @rengolin pointed out.

[sdesmalen]

Renamed tryParseVectorRegister into tryParseNeonVectorRegister.

[rengolin]

This looks good to me, thanks!

Let's wait for a final round of comments.

[fhahn]

Looks good to me now.

lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp

 #include <tuple>
 #include <utility>
 #include <vector>
 
 using namespace llvm;
 
 namespace {
 
+enum class RegKind {Scalar, NeonVector, SVEVector };
+
 class AArch64AsmParser : public MCTargetAsmParser {
 private:
   StringRef Mnemonic; ///< Instruction mnemonic.
 
   // Map of register aliases registers via the .req directive.
-  StringMap<std::pair<bool, unsigned>> RegisterReqs;
+  StringMap<std::pair<RegKind, unsigned>> RegisterReqs;
 
   AArch64TargetStreamer &getTargetStreamer() {
     MCTargetStreamer &TS = *getParser().getStreamer().getTargetStreamer();
     return static_cast<AArch64TargetStreamer &>(TS);
   }
 
   SMLoc getLoc() const { return getParser().getTok().getLoc(); }
 
   bool parseSysAlias(StringRef Name, SMLoc NameLoc, OperandVector &Operands);
   void createSysAlias(uint16_t Encoding, OperandVector &Operands, SMLoc S);
   AArch64CC::CondCode parseCondCodeString(StringRef Cond);
   bool parseCondCode(OperandVector &Operands, bool invertCondCode);
+  unsigned matchReqRegisterNameAlias(StringRef Name, RegKind Kind);
   unsigned matchRegisterNameAlias(StringRef Name, bool isVector);
   int tryParseRegister();
   int tryMatchVectorRegister(StringRef &Kind, bool expected);
   bool parseRegister(OperandVector &Operands);
   bool parseSymbolicImmVal(const MCExpr *&ImmVal);
   bool parseVectorList(OperandVector &Operands);
   bool parseOperand(OperandVector &Operands, bool isCondCode,
                     bool invertCondCode);
private:
   struct TokOp {
     const char *Data;
     unsigned Length;
     bool IsSuffix; // Is the operand actually a suffix on the mnemonic.
   };
 
   struct RegOp {
     unsigned RegNum;
-    bool isVector;
+    RegKind Kind;
   };
 
   struct VectorListOp {
     unsigned RegNum;
     unsigned Count;
     unsigned NumElements;
     unsigned ElementKind;
   };
return (SysReg.PStateField == AArch64PState::PAN ||
             SysReg.PStateField == AArch64PState::UAO);
   }
 
   bool isSystemPStateFieldWithImm0_15() const {
     if (!isSysReg() || isSystemPStateFieldWithImm0_1()) return false;
     return SysReg.PStateField != -1U;
   }
 
-  bool isReg() const override { return Kind == k_Register && !Reg.isVector; }
-  bool isVectorReg() const { return Kind == k_Register && Reg.isVector; }
+  bool isReg() const override {
+    return Kind == k_Register && Reg.Kind == RegKind::Scalar;
+  }
+  bool isVectorReg() const {
+    return Kind == k_Register && Reg.Kind == RegKind::NeonVector;
+  }
 
   bool isVectorRegLo() const {
-    return Kind == k_Register && Reg.isVector &&
+    return Kind == k_Register && Reg.Kind == RegKind::NeonVector &&
            AArch64MCRegisterClasses[AArch64::FPR128_loRegClassID].contains(
                Reg.RegNum);
   }
 
   bool isGPR32as64() const {
-    return Kind == k_Register && !Reg.isVector &&
+    return Kind == k_Register && Reg.Kind == RegKind::Scalar &&
       AArch64MCRegisterClasses[AArch64::GPR64RegClassID].contains(Reg.RegNum);
   }
 
   bool isWSeqPair() const {
-    return Kind == k_Register && !Reg.isVector &&
+    return Kind == k_Register && Reg.Kind == RegKind::Scalar &&
            AArch64MCRegisterClasses[AArch64::WSeqPairsClassRegClassID].contains(
                Reg.RegNum);
   }
 
   bool isXSeqPair() const {
-    return Kind == k_Register && !Reg.isVector &&
+    return Kind == k_Register && Reg.Kind == RegKind::Scalar &&
            AArch64MCRegisterClasses[AArch64::XSeqPairsClassRegClassID].contains(
                Reg.RegNum);
   }
 
   bool isGPR64sp0() const {
-    return Kind == k_Register && !Reg.isVector &&
+    return Kind == k_Register && Reg.Kind == RegKind::Scalar &&
       AArch64MCRegisterClasses[AArch64::GPR64spRegClassID].contains(Reg.RegNum);
   }
 
   template<int64_t Angle, int64_t Remainder>
   bool isComplexRotation() const {
     if (!isImm()) return false;
 
     const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
CreateToken(StringRef Str, bool IsSuffix, SMLoc S, MCContext &Ctx) {
     Op->Tok.Length = Str.size();
     Op->Tok.IsSuffix = IsSuffix;
     Op->StartLoc = S;
     Op->EndLoc = S;
     return Op;
   }
 
   static std::unique_ptr<AArch64Operand>
-  CreateReg(unsigned RegNum, bool isVector, SMLoc S, SMLoc E, MCContext &Ctx) {
+  CreateReg(unsigned RegNum, RegKind Kind, SMLoc S, SMLoc E, MCContext &Ctx) {
     auto Op = make_unique<AArch64Operand>(k_Register, Ctx);
     Op->Reg.RegNum = RegNum;
-    Op->Reg.isVector = isVector;
+    Op->Reg.Kind = Kind;
     Op->StartLoc = S;
     Op->EndLoc = E;
     return Op;
   }
 
   static std::unique_ptr<AArch64Operand>
   CreateVectorList(unsigned RegNum, unsigned Count, unsigned NumElements,
                    char ElementKind, SMLoc S, SMLoc E, MCContext &Ctx) {

 bool AArch64AsmParser::ParseRegister(unsigned &RegNo, SMLoc &StartLoc,
                                      SMLoc &EndLoc) {
   StartLoc = getLoc();
   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,
-                                                  bool isVector) {
-  unsigned RegNum = isVector ? matchVectorRegName(Name)
-                             : MatchRegisterName(Name);
-
-  if (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())
-      return 0;
-    // set RegNum if the match is the right kind of register
-    if (isVector == Entry->getValue().first)
-      RegNum = Entry->getValue().second;
-  }
+unsigned AArch64AsmParser::matchReqRegisterNameAlias(StringRef Name,
+                                                     RegKind Kind) {
+  if (Kind == Entry->getValue().first)
+    return Entry->getValue().second;
+  return 0;
+
+// Matches a register name or register alias previously defined by '.req'
+unsigned AArch64AsmParser::matchRegisterNameAlias(StringRef Name,
+                                                  bool isVector) {
+  unsigned RegNum = isVector ? matchVectorRegName(Name)
+                             : MatchRegisterName(Name);
+  RegKind Kind = isVector ? RegKind::NeonVector : RegKind::Scalar;
+
+  if (RegNum == 0)
+    RegNum = matchReqRegisterNameAlias (Name, Kind);
   return RegNum;
 }
 
 /// tryParseRegister - Try to parse a register name. The token must be an
 /// 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;

 }
 
 /// tryParseVectorRegister - Parse a vector register operand.
 bool AArch64AsmParser::tryParseVectorRegister(OperandVector &Operands) {
   MCAsmParser &Parser = getParser();
   if (Parser.getTok().isNot(AsmToken::Identifier))
     return true;
 
+  RegKind RegisterKind = RegKind::NeonVector;
   SMLoc S = getLoc();
   // Check for a vector register specifier first.
   StringRef Kind;
   int64_t Reg = tryMatchVectorRegister(Kind, false);
   if (Reg == -1)
     return true;
   Operands.push_back(
-      AArch64Operand::CreateReg(Reg, true, S, getLoc(), getContext()));
+      AArch64Operand::CreateReg(Reg, RegisterKind, S, getLoc(), getContext()));
+
   // If there was an explicit qualifier, that goes on as a literal text
   // operand.
   if (!Kind.empty())
     Operands.push_back(
         AArch64Operand::CreateToken(Kind, false, S, getContext()));
 
   // If there is an index specifier following the register, parse that too.
   SMLoc SIdx = getLoc();
bool AArch64AsmParser::tryParseVectorRegister(OperandVector &Operands) {
   }
 
   return false;
 }
 
 /// parseRegister - Parse a non-vector register operand.
 bool AArch64AsmParser::parseRegister(OperandVector &Operands) {
   SMLoc S = getLoc();
-  // Try for a vector register.
+  // Try for a vector (neon) register.
   if (!tryParseVectorRegister(Operands))
     return false;
 
   // Try for a scalar register.
   int64_t Reg = tryParseRegister();
   if (Reg == -1)
     return true;
-  Operands.push_back(
-      AArch64Operand::CreateReg(Reg, false, S, getLoc(), getContext()));
+  Operands.push_back(AArch64Operand::CreateReg(Reg, RegKind::Scalar, S,
+      getLoc(), getContext()));
 
   return false;
 }
 
 bool AArch64AsmParser::parseSymbolicImmVal(const MCExpr *&ImmVal) {
   MCAsmParser &Parser = getParser();
   bool HasELFModifier = false;
   AArch64MCExpr::VariantKind RefKind;
if (Count > 4)
     return Error(S, "invalid number of vectors");
 
   unsigned NumElements = 0;
   char ElementKind = 0;
   if (!Kind.empty())
     parseValidVectorKind(Kind, NumElements, ElementKind);
 
   Operands.push_back(AArch64Operand::CreateVectorList(
-      FirstReg, Count, NumElements, ElementKind, S, getLoc(), getContext()));
+      FirstReg, Count, NumElements, ElementKind, S, getLoc(),
+      getContext()));
 
   // If there is an index specifier following the list, parse that too.
   SMLoc SIdx = getLoc();
   if (parseOptionalToken(AsmToken::LBrac)) { // Eat left bracket token.
     const MCExpr *ImmVal;
     if (getParser().parseExpression(ImmVal))
       return false;
     const MCConstantExpr *MCE = dyn_cast<MCConstantExpr>(ImmVal);
AArch64AsmParser::tryParseGPR64sp0Operand(OperandVector &Operands) {
   if (!RI->getRegClass(AArch64::GPR64spRegClassID).contains(RegNum))
     return MatchOperand_NoMatch;
 
   SMLoc S = getLoc();
   Parser.Lex(); // Eat register
 
   if (!parseOptionalToken(AsmToken::Comma)) {
     Operands.push_back(
-        AArch64Operand::CreateReg(RegNum, false, S, getLoc(), Ctx));
+        AArch64Operand::CreateReg(RegNum, RegKind::Scalar, S, getLoc(), Ctx));
     return MatchOperand_Success;
   }
 
   parseOptionalToken(AsmToken::Hash);
 
   if (Parser.getTok().isNot(AsmToken::Integer)) {
     Error(getLoc(), "index must be absent or #0");
     return MatchOperand_ParseFail;
   }
 
   const MCExpr *ImmVal;
   if (Parser.parseExpression(ImmVal) || !isa<MCConstantExpr>(ImmVal) ||
       cast<MCConstantExpr>(ImmVal)->getValue() != 0) {
     Error(getLoc(), "index must be absent or #0");
     return MatchOperand_ParseFail;
   }
 
   Operands.push_back(
-      AArch64Operand::CreateReg(RegNum, false, S, getLoc(), Ctx));
+      AArch64Operand::CreateReg(RegNum, RegKind::Scalar, S, getLoc(), Ctx));
   return MatchOperand_Success;
 }
 
 /// parseOperand - Parse a arm instruction operand.  For now this parses the
 /// operand regardless of the mnemonic.
 bool AArch64AsmParser::parseOperand(OperandVector &Operands, bool isCondCode,
                                   bool invertCondCode) {
   MCAsmParser &Parser = getParser();
if (Op1.isReg() && LSBOp.isImm() && WidthOp.isImm()) {
           return Error(WidthOp.getStartLoc(),
                        "requested insert overflows register");
 
         const MCExpr *ImmRExpr = MCConstantExpr::create(ImmR, getContext());
         const MCExpr *ImmSExpr = MCConstantExpr::create(ImmS, getContext());
         Operands[0] = AArch64Operand::CreateToken(
               "bfm", false, Op.getStartLoc(), getContext());
         Operands[2] = AArch64Operand::CreateReg(
-            RegWidth == 32 ? AArch64::WZR : AArch64::XZR, false, SMLoc(),
-            SMLoc(), getContext());
+            RegWidth == 32 ? AArch64::WZR : AArch64::XZR, RegKind::Scalar,
+            SMLoc(), SMLoc(), getContext());
         Operands[3] = AArch64Operand::CreateImm(
             ImmRExpr, LSBOp.getStartLoc(), LSBOp.getEndLoc(), getContext());
         Operands.emplace_back(
             AArch64Operand::CreateImm(ImmSExpr, WidthOp.getStartLoc(),
                                       WidthOp.getEndLoc(), getContext()));
       }
     }
   } else if (NumOperands == 5) {
bool AArch64AsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
   //        InstAlias can't quite handle this since the reg classes aren't
   //        subclasses.
   if (NumOperands == 3 && (Tok == "sxtw" || Tok == "uxtw")) {
     // The source register can be Wn here, but the matcher expects a
     // GPR64. Twiddle it here if necessary.
     AArch64Operand &Op = static_cast<AArch64Operand &>(*Operands[2]);
     if (Op.isReg()) {
       unsigned Reg = getXRegFromWReg(Op.getReg());
-      Operands[2] = AArch64Operand::CreateReg(Reg, false, Op.getStartLoc(),
-                                              Op.getEndLoc(), getContext());
+      Operands[2] = AArch64Operand::CreateReg(Reg, RegKind::Scalar,
+                                              Op.getStartLoc(), Op.getEndLoc(),
+                                              getContext());
     }
   }
   // FIXME: Likewise for sxt[bh] with a Xd dst operand
   else if (NumOperands == 3 && (Tok == "sxtb" || Tok == "sxth")) {
     AArch64Operand &Op = static_cast<AArch64Operand &>(*Operands[1]);
     if (Op.isReg() &&
         AArch64MCRegisterClasses[AArch64::GPR64allRegClassID].contains(
             Op.getReg())) {
       // The source register can be Wn here, but the matcher expects a
       // GPR64. Twiddle it here if necessary.
       AArch64Operand &Op = static_cast<AArch64Operand &>(*Operands[2]);
       if (Op.isReg()) {
         unsigned Reg = getXRegFromWReg(Op.getReg());
-        Operands[2] = AArch64Operand::CreateReg(Reg, false, Op.getStartLoc(),
+        Operands[2] = AArch64Operand::CreateReg(Reg, RegKind::Scalar,
+                                                Op.getStartLoc(),
                                                 Op.getEndLoc(), getContext());
       }
     }
   }
   // FIXME: Likewise for uxt[bh] with a Xd dst operand
   else if (NumOperands == 3 && (Tok == "uxtb" || Tok == "uxth")) {
     AArch64Operand &Op = static_cast<AArch64Operand &>(*Operands[1]);
     if (Op.isReg() &&
         AArch64MCRegisterClasses[AArch64::GPR64allRegClassID].contains(
             Op.getReg())) {
       // The source register can be Wn here, but the matcher expects a
       // GPR32. Twiddle it here if necessary.
       AArch64Operand &Op = static_cast<AArch64Operand &>(*Operands[1]);
       if (Op.isReg()) {
         unsigned Reg = getWRegFromXReg(Op.getReg());
-        Operands[1] = AArch64Operand::CreateReg(Reg, false, Op.getStartLoc(),
+        Operands[1] = AArch64Operand::CreateReg(Reg, RegKind::Scalar,
+                                                Op.getStartLoc(),
                                                 Op.getEndLoc(), getContext());
       }
     }
   }
 
   MCInst Inst;
   // First try to match against the secondary set of tables containing the
   // short-form NEON instructions (e.g. "fadd.2s v0, v1, v2").

 
 /// parseDirectiveReq
 ///  ::= name .req registername
 bool AArch64AsmParser::parseDirectiveReq(StringRef Name, SMLoc L) {
   MCAsmParser &Parser = getParser();
   Parser.Lex(); // Eat the '.req' token.
   SMLoc SRegLoc = getLoc();
   unsigned RegNum = tryParseRegister();
-  bool IsVector = false;
+  RegKind RegisterKind = RegKind::Scalar;
 
   if (RegNum == static_cast<unsigned>(-1)) {
     StringRef Kind;
+    RegisterKind = RegKind::NeonVector;
     RegNum = tryMatchVectorRegister(Kind, false);
     if (!Kind.empty())
       return Error(SRegLoc, "vector register without type specifier expected");
-    IsVector = true;
   }
 
   if (RegNum == static_cast<unsigned>(-1))
     return Error(SRegLoc, "register name or alias expected");
 
   // Shouldn't be anything else.
   if (parseToken(AsmToken::EndOfStatement,
                  "unexpected input in .req directive"))
     return true;
 
-  auto pair = std::make_pair(IsVector, RegNum);
+  auto pair = std::make_pair(RegisterKind, RegNum);
   if (RegisterReqs.insert(std::make_pair(Name, pair)).first->second != pair)
     Warning(L, "ignoring redefinition of register alias '" + Name + "'");
 
   return false;
 }
 
 /// parseDirectiveUneq
 ///  ::= .unreq registername
AArch64AsmParser::tryParseGPRSeqPair(OperandVector &Operands) {
   if (isXReg) {
     Pair = RI->getMatchingSuperReg(FirstReg, AArch64::sube64,
            &AArch64MCRegisterClasses[AArch64::XSeqPairsClassRegClassID]);
   } else {
     Pair = RI->getMatchingSuperReg(FirstReg, AArch64::sube32,
            &AArch64MCRegisterClasses[AArch64::WSeqPairsClassRegClassID]);
   }
 
-  Operands.push_back(AArch64Operand::CreateReg(Pair, false, S, getLoc(),
-      getContext()));
+  Operands.push_back(AArch64Operand::CreateReg(Pair, RegKind::Scalar, S,
+      getLoc(), getContext()));
 
   return MatchOperand_Success;
 }