[RISCV] Use custom operand parsing for FenceArg.

[craig.topper]

Summary

Rather than parsing a symbol and checking its name, look for an
identifier during parsing.

I've also handled the 0 immediate during parsing, though we could
let that go through the normal immediate parsing if we need to support
expressions that evaluate to 0. We don't have tests for that currently.

[frasercrmck]

LGTM other than nits

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1850

I don't think you need the braces in this statement because Valid was declared outside of the switch scope?

1876

Not sure it's worth it but having three uses of the exact same string made me wonder if this should be a shared variable.

[luismarques]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1854–1865

In a follow-up patch, we should reject repeated letters, like iiorrrrrw. Binutils seems to reject that, which IMO is the right decision.

[frasercrmck]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1854–1865

I took the c <= Prev check below to do just that

[luismarques]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1854–1865

Oops, for some reason I didn't notice that. Thanks, @frasercrmck.
The tests should be extended to cover this tighter checking, though.
The patch summary should also mention that change. I'm not sure if that is noteworthy enough for release notes? It is a user-visible change...

[asb]

Nice cleanup - thanks!

IMHO a mention in the release notes is not necessary for such a minor change to accepted input.

[craig.topper]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1854–1865

That code isn’t new. It was already in isFenceArg in the old code.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

 //===-- RISCVAsmParser.cpp - Parse RISCV assembly to MCInst instructions --===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
#include "RISCVGenAsmMatcher.inc"
   OperandMatchResultTy parseCallSymbol(OperandVector &Operands);
   OperandMatchResultTy parsePseudoJumpSymbol(OperandVector &Operands);
   OperandMatchResultTy parseJALOffset(OperandVector &Operands);
   OperandMatchResultTy parseVTypeI(OperandVector &Operands);
   OperandMatchResultTy parseMaskReg(OperandVector &Operands);
   OperandMatchResultTy parseInsnDirectiveOpcode(OperandVector &Operands);
   OperandMatchResultTy parseGPRAsFPR(OperandVector &Operands);
   OperandMatchResultTy parseFRMArg(OperandVector &Operands);
+  OperandMatchResultTy parseFenceArg(OperandVector &Operands);
 
   bool parseOperand(OperandVector &Operands, StringRef Mnemonic);
 
   bool parseDirectiveOption();
   bool parseDirectiveAttribute();
   bool parseDirectiveInsn(SMLoc L);
   bool parseDirectiveVariantCC();
 
struct RISCVOperand final : public MCParsedAsmOperand {
 
   enum class KindTy {
     Token,
     Register,
     Immediate,
     SystemRegister,
     VType,
     FRM,
+    Fence,
   } Kind;
 
   struct RegOp {
     MCRegister RegNum;
     bool IsRV64;
     bool IsGPRAsFPR;
   };
 
struct RISCVOperand final : public MCParsedAsmOperand {
   struct VTypeOp {
     unsigned Val;
   };
 
   struct FRMOp {
     RISCVFPRndMode::RoundingMode FRM;
   };
 
+  struct FenceOp {
+    unsigned Val;
+  };
+
   SMLoc StartLoc, EndLoc;
   union {
     StringRef Tok;
     RegOp Reg;
     ImmOp Imm;
     struct SysRegOp SysReg;
     struct VTypeOp VType;
     struct FRMOp FRM;
+    struct FenceOp Fence;
   };
 
   RISCVOperand(KindTy K) : Kind(K) {}
 
 public:
   RISCVOperand(const RISCVOperand &o) : MCParsedAsmOperand() {
     Kind = o.Kind;
     StartLoc = o.StartLoc;
case KindTy::SystemRegister:
       SysReg = o.SysReg;
       break;
     case KindTy::VType:
       VType = o.VType;
       break;
     case KindTy::FRM:
       FRM = o.FRM;
       break;
+    case KindTy::Fence:
+      Fence = o.Fence;
+      break;
     }
   }
 
   bool isToken() const override { return Kind == KindTy::Token; }
   bool isReg() const override { return Kind == KindTy::Register; }
   bool isV0Reg() const {
     return Kind == KindTy::Register && Reg.RegNum == RISCV::V0;
   }
public:
   bool isVTypeI11() const {
     if (Kind == KindTy::Immediate)
       return isVTypeImm(11);
     return Kind == KindTy::VType;
   }
 
   /// Return true if the operand is a valid for the fence instruction e.g.
   /// ('iorw').
-  bool isFenceArg() const {
-    if (!isImm())
-      return false;
-
-    int64_t Imm;
-    RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
-    if (evaluateConstantImm(getImm(), Imm, VK)) {
-      // Only accept 0 as a constant immediate.
-      return VK == RISCVMCExpr::VK_RISCV_None && Imm == 0;
-    }
-
-    auto *SVal = dyn_cast<MCSymbolRefExpr>(getImm());
-
-    if (!SVal || SVal->getKind() != MCSymbolRefExpr::VK_None)
-      return false;
-
-    StringRef Str = SVal->getSymbol().getName();
-    // Letters must be unique, taken from 'iorw', and in ascending order. This
-    // holds as long as each individual character is one of 'iorw' and is
-    // greater than the previous character.
-    char Prev = '\0';
-    for (char c : Str) {
-      if (c != 'i' && c != 'o' && c != 'r' && c != 'w')
-        return false;
-      if (c <= Prev)
-        return false;
-      Prev = c;
-    }
-    return true;
-  }
+  bool isFenceArg() const { return Kind == KindTy::Fence; }
 
   /// Return true if the operand is a valid floating point rounding mode.
   bool isFRMArg() const { return Kind == KindTy::FRM; }
 
   bool isImmXLenLI() const {
     int64_t Imm;
     RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     if (!isImm())
unsigned getVType() const {
     return VType.Val;
   }
 
   RISCVFPRndMode::RoundingMode getFRM() const {
     assert(Kind == KindTy::FRM && "Invalid type access!");
     return FRM.FRM;
   }
 
+  unsigned getFence() const {
+    assert(Kind == KindTy::Fence && "Invalid type access!");
+    return Fence.Val;
+  }
+
   void print(raw_ostream &OS) const override {
     auto RegName = [](MCRegister Reg) {
       if (Reg)
         return RISCVInstPrinter::getRegisterName(Reg);
       else
         return "noreg";
     };
 
case KindTy::VType:
       RISCVVType::printVType(getVType(), OS);
       OS << '>';
       break;
     case KindTy::FRM:
       OS << "<frm: ";
       roundingModeToString(getFRM());
       OS << '>';
       break;
+    case KindTy::Fence:
+      OS << "<fence: ";
+      OS << getFence();
+      OS << '>';
+      break;
     }
   }
 
   static std::unique_ptr<RISCVOperand> createToken(StringRef Str, SMLoc S) {
     auto Op = std::make_unique<RISCVOperand>(KindTy::Token);
     Op->Tok = Str;
     Op->StartLoc = S;
     Op->EndLoc = S;
public:
   createFRMArg(RISCVFPRndMode::RoundingMode FRM, SMLoc S) {
     auto Op = std::make_unique<RISCVOperand>(KindTy::FRM);
     Op->FRM.FRM = FRM;
     Op->StartLoc = S;
     Op->EndLoc = S;
     return Op;
   }
 
+  static std::unique_ptr<RISCVOperand> createFenceArg(unsigned Val, SMLoc S) {
+    auto Op = std::make_unique<RISCVOperand>(KindTy::Fence);
+    Op->Fence.Val = Val;
+    Op->StartLoc = S;
+    Op->EndLoc = S;
+    return Op;
+  }
+
   static std::unique_ptr<RISCVOperand> createVType(unsigned VTypeI, SMLoc S) {
     auto Op = std::make_unique<RISCVOperand>(KindTy::VType);
     Op->VType.Val = VTypeI;
     Op->StartLoc = S;
     Op->EndLoc = S;
     return Op;
   }
 
public:
 
   void addImmOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     addExpr(Inst, getImm());
   }
 
   void addFenceArgOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
-
-    int64_t Constant = 0;
-    RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
-    if (evaluateConstantImm(getImm(), Constant, VK)) {
-      if (Constant == 0) {
-        Inst.addOperand(MCOperand::createImm(Constant));
-        return;
-      }
-      llvm_unreachable("FenceArg must contain only [iorw] or be 0");
-    }
-
-    // isFenceArg has validated the operand, meaning this cast is safe
-    auto SE = cast<MCSymbolRefExpr>(getImm());
-
-    unsigned Imm = 0;
-    for (char c : SE->getSymbol().getName()) {
-      switch (c) {
-      default:
-        llvm_unreachable("FenceArg must contain only [iorw] or be 0");
-      case 'i':
-        Imm |= RISCVFenceField::I;
-        break;
-      case 'o':
-        Imm |= RISCVFenceField::O;
-        break;
-      case 'r':
-        Imm |= RISCVFenceField::R;
-        break;
-      case 'w':
-        Imm |= RISCVFenceField::W;
-        break;
-      }
-    }
-    Inst.addOperand(MCOperand::createImm(Imm));
+    Inst.addOperand(MCOperand::createImm(Fence.Val));
   }
 
   void addCSRSystemRegisterOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     Inst.addOperand(MCOperand::createImm(SysReg.Encoding));
   }
 
   // Support non-canonical syntax:
case Match_InvalidSImm21Lsb0JAL:
     return generateImmOutOfRangeError(
         Operands, ErrorInfo, -(1 << 20), (1 << 20) - 2,
         "immediate must be a multiple of 2 bytes in the range");
   case Match_InvalidCSRSystemRegister: {
     return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 12) - 1,
                                       "operand must be a valid system register "
                                       "name or an integer in the range");
   }
-  case Match_InvalidFenceArg: {
-    SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
-    return Error(ErrorLoc, "operand must be formed of letters selected "
-                           "in-order from 'iorw' or be 0");
-  }
   case Match_InvalidBareSymbol: {
     SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
     return Error(ErrorLoc, "operand must be a bare symbol name");
   }
   case Match_InvalidPseudoJumpSymbol: {
     SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
     return Error(ErrorLoc, "operand must be a valid jump target");
   }
if (FRM == RISCVFPRndMode::Invalid) {
     return MatchOperand_ParseFail;
   }
 
   Operands.push_back(RISCVOperand::createFRMArg(FRM, getLoc()));
   Lex(); // Eat identifier token.
   return MatchOperand_Success;
 }
 
+OperandMatchResultTy RISCVAsmParser::parseFenceArg(OperandVector &Operands) {
+  const AsmToken &Tok = getLexer().getTok();
+
+  if (Tok.is(AsmToken::Integer)) {
+    if (Tok.getIntVal() != 0) {
+      TokError(
+          "operand must be formed of letters selected in-order from 'iorw' "
+          "or be 0");
+      return MatchOperand_ParseFail;
+    }
+
+    Operands.push_back(RISCVOperand::createFenceArg(0, getLoc()));
+    Lex();
+    return MatchOperand_Success;
+  }
+
+  if (Tok.isNot(AsmToken::Identifier)) {
+    TokError("operand must be formed of letters selected in-order from 'iorw' "
+             "or be 0");
+    return MatchOperand_ParseFail;
+  }
+
+  StringRef Str = Tok.getIdentifier();
+
+  // Letters must be unique, taken from 'iorw', and in ascending order. This
+  // holds as long as each individual character is one of 'iorw' and is
+  // greater than the previous character.
+  unsigned Imm = 0;
+  bool Valid = true;
+  char Prev = '\0';
+  for (char c : Str) {
+    switch (c) {
+    default: {
+      Valid = false;
+      break;
+    }
+    case 'i':
+      Imm |= RISCVFenceField::I;
+      break;
+    case 'o':
+      Imm |= RISCVFenceField::O;
+      break;
+    case 'r':
+      Imm |= RISCVFenceField::R;
+      break;
+    case 'w':
+      Imm |= RISCVFenceField::W;
+      break;
+    }
+
+    if (c <= Prev) {
+      Valid = false;
+      break;
+    }
+    Prev = c;
+  }
+
+  if (!Valid) {
+    TokError("operand must be formed of letters selected in-order from 'iorw' "
+             "or be 0");
+    return MatchOperand_ParseFail;
+  }
+
+  Operands.push_back(RISCVOperand::createFenceArg(Imm, getLoc()));
+  Lex();
+  return MatchOperand_Success;
+}
+
 OperandMatchResultTy
 RISCVAsmParser::parseMemOpBaseReg(OperandVector &Operands) {
   if (getLexer().isNot(AsmToken::LParen)) {
     Error(getLoc(), "expected '('");
     return MatchOperand_ParseFail;
   }
 
   getParser().Lex(); // Eat '('

llvm/lib/Target/RISCV/RISCVInstrInfo.td

 
 class UImmAsmOperand<int width, string suffix = "">
     : ImmAsmOperand<"U", width, suffix> {
 }
 
 def FenceArg : AsmOperandClass {
   let Name = "FenceArg";
   let RenderMethod = "addFenceArgOperands";
-  let DiagnosticType = "InvalidFenceArg";
+  let ParserMethod = "parseFenceArg";
 }
 
 def fencearg : Operand<XLenVT> {
   let ParserMatchClass = FenceArg;
   let PrintMethod = "printFenceArg";
   let DecoderMethod = "decodeUImmOperand<4>";
   let OperandType = "OPERAND_UIMM4";
   let OperandNamespace = "RISCVOp";