[RISCV] Clean up parsing fence arguments

[jrtc27]

Summary

Currently this is done rather hackily by letting it be parsed as an
MCSymbolRefExpr whose name we then validate. The logic is also split
across isFenceArg and addFenceArgOperands as a result. Instead, use a
custom parser so we look at the identifier token directly and construct
the immediate at the same time. As well as being cleaner, this also
allows us to give better error messages.

[craig.topper]

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

1688

I see this SMLoc::getFromPointer(S.getPointer() - 1) repeated a lot in the assembly parser. Is this doing something I don't understand and setting a valid end location or are we just frequently setting an invalid end location?

[jrtc27]

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

1688

Hm, indeed, this seems to be a consistent misunderstanding in the parser here. Normally you'd lex and _then_ get E based on the _new_ location, but this creates a -1-sized range. The -1 itself is also odd, though seems to be rather pervasive across the backends; SMRange is meant to be half-open, so the -1 is already accounted for.

[craig.topper]

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

1688

You don't even need to Lex the next token. There's a getEndLoc() on the Token class.

[jrtc27]

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

1688

In this case, yeah, though some cases in this file are more complex and so would likely need to use that kind of pattern instead.

[luismarques]

LGTM.

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

1706–1717

Keep the original in-line formatting?

llvm/test/MC/RISCV/rv32i-invalid.s

5–7

letters -> operand letters?

9

ditto.

[asb]

Great cleanup, thanks for this.

[jrtc27]

Obsoleted by d558a70abf2624bf0b26fa8cd506277214fea197

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

 #include "RISCVGenAsmMatcher.inc"
 
   OperandMatchResultTy parseCSRSystemRegister(OperandVector &Operands);
   OperandMatchResultTy parseImmediate(OperandVector &Operands);
   OperandMatchResultTy parseRegister(OperandVector &Operands,
                                      bool AllowParens = false);
   OperandMatchResultTy parseMemOpBaseReg(OperandVector &Operands);
   OperandMatchResultTy parseAtomicMemOp(OperandVector &Operands);
+  OperandMatchResultTy parseFenceArg(OperandVector &Operands);
   OperandMatchResultTy parseOperandWithModifier(OperandVector &Operands);
   OperandMatchResultTy parseBareSymbol(OperandVector &Operands);
   OperandMatchResultTy parseCallSymbol(OperandVector &Operands);
   OperandMatchResultTy parsePseudoJumpSymbol(OperandVector &Operands);
   OperandMatchResultTy parseJALOffset(OperandVector &Operands);
   OperandMatchResultTy parseVTypeI(OperandVector &Operands);
   OperandMatchResultTy parseMaskReg(OperandVector &Operands);
 
bool isTPRelAddSymbol() const {
     return RISCVAsmParser::classifySymbolRef(getImm(), VK) &&
            VK == RISCVMCExpr::VK_RISCV_TPREL_ADD;
   }
 
   bool isCSRSystemRegister() const { return isSystemRegister(); }
 
   bool isVTypeI() const { return isVType(); }
 
-  /// Return true if the operand is a valid for the fence instruction e.g.
-  /// ('iorw').
   bool isFenceArg() const {
+    int64_t Imm;
+    RISCVMCExpr::VariantKind VK = RISCVMCExpr::VK_RISCV_None;
     if (!isImm())
       return false;
-    const MCExpr *Val = getImm();
-    auto *SVal = dyn_cast<MCSymbolRefExpr>(Val);
-    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 IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+    if (!IsConstantImm)
+    int64_t Mask = RISCVFenceField::I | RISCVFenceField::O |
+                   RISCVFenceField::R | RISCVFenceField::W;
+    return (Imm & ~Mask) == 0;
   }
 
   /// Return true if the operand is a valid floating point rounding mode.
   bool isFRMArg() const {
     if (!isImm())
       return false;
     const MCExpr *Val = getImm();
     auto *SVal = dyn_cast<MCSymbolRefExpr>(Val);
void addRegOperands(MCInst &Inst, unsigned N) const {
     Inst.addOperand(MCOperand::createReg(getReg()));
   }
 
   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!");
-    // 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]");
-      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));
-  }
-
   void addCSRSystemRegisterOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     Inst.addOperand(MCOperand::createImm(SysReg.Encoding));
   }
 
   void addVTypeIOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     Inst.addOperand(MCOperand::createImm(getVType()));
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'");
-  }
   case Match_InvalidFRMArg: {
     SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
     return Error(
         ErrorLoc,
         "operand must be a valid floating point rounding mode mnemonic");
   }
   case Match_InvalidBareSymbol: {
     SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
if (OptionalImmOp && !OptionalImmOp->isImmZero()) {
     Error(OptionalImmOp->getStartLoc(), "optional integer offset must be 0",
           SMRange(OptionalImmOp->getStartLoc(), OptionalImmOp->getEndLoc()));
     return MatchOperand_ParseFail;
   }
 
   return MatchOperand_Success;
 }
 
+OperandMatchResultTy RISCVAsmParser::parseFenceArg(OperandVector &Operands) {
+  SMLoc S = getLoc();
+  SMLoc E = SMLoc::getFromPointer(S.getPointer() - 1);
+  const MCExpr *Res;
+
+  if (getLexer().getKind() != AsmToken::Identifier) {
+    Error(S, "operand must be formed of letters selected in-order from 'iorw'");
+    return MatchOperand_ParseFail;
+  }
+
+  StringRef Identifier = getParser().getTok().getIdentifier();
+  getParser().Lex();
+
+  // 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';
+  unsigned Imm = 0;
+  for (char c : Identifier) {
+    switch (c) {
+    case 'i':
+      Imm |= RISCVFenceField::I;
+      break;
+    case 'o':
+      Imm |= RISCVFenceField::O;
+      break;
+    case 'r':
+      Imm |= RISCVFenceField::R;
+      break;
+    case 'w':
+      Imm |= RISCVFenceField::W;
+      break;
+    default:
+      Error(S, "letters must be selected from 'iorw'");
+      return MatchOperand_ParseFail;
+    }
+
+    if (c <= Prev) {
+      if (c == Prev)
+        Error(S, "letters must not be duplicated");
+      else
+        Error(S, "letters must be in the order 'iorw'");
+      return MatchOperand_ParseFail;
+    }
+    Prev = c;
+  }
+
+  Res = MCConstantExpr::create(Imm, getContext());
+  Operands.push_back(RISCVOperand::createImm(Res, S, E, isRV64()));
+  return MatchOperand_Success;
+}
+
 /// Looks at a token type and creates the relevant operand from this
 /// information, adding to Operands. If operand was parsed, returns false, else
 /// true.
 bool RISCVAsmParser::parseOperand(OperandVector &Operands, StringRef Mnemonic) {
   // Check if the current operand has a custom associated parser, if so, try to
   // custom parse the operand, or fallback to the general approach.
   OperandMatchResultTy Result =
       MatchOperandParserImpl(Operands, Mnemonic, /*ParseForAllFeatures=*/true);

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 RenderMethod = "addImmOperands";
+  let ParserMethod = "parseFenceArg";
 }
 
 def fencearg : Operand<XLenVT> {
   let ParserMatchClass = FenceArg;
   let PrintMethod = "printFenceArg";
   let DecoderMethod = "decodeUImmOperand<4>";
   let OperandType = "OPERAND_UIMM4";
   let OperandNamespace = "RISCVOp";

llvm/test/MC/RISCV/rv32i-invalid.s

 # RUN: not llvm-mc -triple riscv32 < %s 2>&1 | FileCheck %s
 
 # Out of range immediates
 ## fencearg
-fence iorw, iore # CHECK: :[[@LINE]]:13: error: operand must be formed of letters selected in-order from 'iorw'
-fence wr, wr # CHECK: :[[@LINE]]:7: error: operand must be formed of letters selected in-order from 'iorw'
-fence rw, rr # CHECK: :[[@LINE]]:11: error: operand must be formed of letters selected in-order from 'iorw'
+fence iorw, iore # CHECK: :[[@LINE]]:13: error: letters must be selected from 'iorw'
+fence wr, wr # CHECK: :[[@LINE]]:7: error: letters must be in the order 'iorw'
+fence rw, rr # CHECK: :[[@LINE]]:11: error: letters must not be duplicated
 fence 1, rw # CHECK: :[[@LINE]]:7: error: operand must be formed of letters selected in-order from 'iorw'
-fence unknown, unknown # CHECK: :[[@LINE]]:7: error: operand must be formed of letters selected in-order from 'iorw'
+fence unknown, unknown # CHECK: :[[@LINE]]:7: error: letters must be selected from 'iorw'
 
 ## uimm5
 slli a0, a0, 32 # CHECK: :[[@LINE]]:14: error: immediate must be an integer in the range [0, 31]
 srli a0, a0, -1 # CHECK: :[[@LINE]]:14: error: immediate must be an integer in the range [0, 31]
 srai a0, a0, -19 # CHECK: :[[@LINE]]:14: error: immediate must be an integer in the range [0, 31]
 csrrwi a1, 0x1, -1 # CHECK: :[[@LINE]]:17: error: immediate must be an integer in the range [0, 31]
 csrrsi t1, 999, 32 # CHECK: :[[@LINE]]:17: error: immediate must be an integer in the range [0, 31]
 csrrci x0, 43, -90 # CHECK: :[[@LINE]]:16: error: immediate must be an integer in the range [0, 31]