[RISCV 8/10] Add support for all RV32I instructions

[asb]

Summary

This is overall, fairly mechanical. Instructions are ordered in the same way as in the RISC-V User-Level ISA Specification.

A future patch will add support for pseudoinstructions and other instruction expansions (e.g. 0-arg fence -> fence iorw, iorw).

[reames]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

146

style: shouldn't this be isImm12?

150

Style: sort these by immediate size.

155

style: duplicated code, possible an isImm<X>? With wrappers potentially?

167

I saw this being discussed in a previous review, but I won't know what these were from the names. Possible a comment? Or a pointer to a design decision?

306

Style: Hard coding these values seems slightly error prone. Could we generate these messages from the immediate size and common all of this code?

lib/Target/RISCV/RISCVInstrInfo.td

130

Shouldn't this simply be two different instructions with disambiguation living in the disassembler?

[asb]

Address comments from @reames. AsmOperand definitions in RISCVInstrInfo have changed. Error reporting code has been commoned. Also describe the CSR instructions that were added in the v2.1 RISC-V ISA spec.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

146

I've reworked things so that tablegen will call isImm12, in fact I also went ahead and moved to UImm and SImm for greater clarity.

155

In D23568 some of the isImm methods get a little more involved. For the isImm methods that are trivial, just having it as a wrapper to a templated function actually help readability? My concern is that it's less easy to see at a glance that a trivial check is taking place rather than something more complex.

167

I've added a comment to the relevant definition in RISCVInstrInfo.td and added a comment to this file that points to the definitions in RISCVInstrInfo.td

306

I've added a common error message generator, which I think is an improvement. I'm not sure whether it's really clearer or not now that the desired range isn't hard-coded.

[theraven]

Comments inline.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

156

This probably wants to be SImmScaled<12,1>(getConstantImm()).

It would also be nice to use the same terminology (scaled immediate) as other back ends and the generic code, rather than simm-mask.

165

As above, should use SImmScaled.

254

If this is only used in this file, it might be better off as a function in an anonymous namespace rather than a method exposed in the header.

306

I agree with reames that it would be nicer to have the ranges come sensibly from TableGen. If you figure out a way to do this, let me know as we are currently specifying the same ranges in three different ways for a few things in the CHERI back end...

[t.p.northover]

I'd expect to see some PrintMethods and InstPrinter adaptations for these (specifically to wrangle the correct immediates from the MCInst representation).

[t.p.northover]

Ah, never mind. Somehow I hadn't realised you were keeping the CodeGen immediate in the MCInst.

[asb]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

156

MIPS uses the SImmScaled functions, but naming is of the form simm19_lsl2. Both the lsl naming and the {U,I}ImmScaled functions are unique to MIPS currently so there's not broad consensus here - though I agree unifying terminology is useful.

I feel the naming is perhaps slightly confusing in that the decision to describe a transformation from the encoded to the 'actual' value seems arbitrary vs describing the transformation from 'actual' value to encoded value. The options I considered were:

• simm20_lsl1:$imm20 (describes how to go from encoded value to logical value. Matches MIPS)
• simm21_asr1:$imm20 (describes going from logical value to encoded value)
• simm21_mask1:$imm20 (current approach, describes the constraints on the encoded value)

Are you suggesting simm20_scaled1:$imm20? Or perhaps simm21_scaled1:$imm20?

[asb]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

156

Or another alternative: given that in the RISC-V ISA the 'scaled' immediates only shift by 1 bit (UJ and SB instruction forms) we could go with simm21_lsb0:$imm20 to indicate that the least significant bit is known 0.

[theraven]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

156

I'm happy with either name, as long as there's a comment explaining what it means where it's first introduced. I'm more concerned to avoid the reimplementation of SImmScaled than what you call the result.

[asb]

Make use of isShiftedInt from MathExtras.h. Rename {simm21,simm13}_mask1 to {simm21,simm13}_lsb0. Tests are updated to check instruction printing.

[asb]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

254

RISCVAsmParser is itself already in an anonymous namespace. Unless I'm misunderstanding the suggestion, I'm not seeing much advantage in splitting out this and other helpers. It also wouldn't match standard practice in other backends.

[asb]

Update test style as suggested by @jyknight in D23564

[jyknight]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

156

(This continues the thread from D23561, but it's more relevant here.)

I find it confusing to talk about the meanings as a transformation. I think the name ought to describe the value itself -- it *is* a 20 bit value, whose meaning is shifted by 1 bit.

Thus, the convention that makes the most sense to me would be to call it an "sImm20s1" -- that is, 20 bits, shifted 1. This is the convention that the arm and aarch64 backends use.

Scaled is an okay word too, except that you might think it's a multiplier, not a shift. (does "scaled2" mean "times 2" or "shifted by 2"?)

Adding "left" or "right" into the name (as in "lsl" or "asr") is seems to me to be unnecessary clarification, that actually adds confusion instead of clarifying. When you specifies direction, I start thinking about what that's trying to say, and about which direction is which, and such. But there's only one sensible direction in the first place, so not saying it somewhat unintuitively seems to be less confusing -- at least for me.

(BTW, since the Aarch64 backend is a pretty new backend, and was written by many of the long-time core contributors to LLVM, I tend to look at it to guide style in preference to other backends. Of course it's not 100% the case that it's always doing things the best way, but I think it's probably more likely than others at the moment.)

lib/Target/RISCV/RISCVInstrInfo.td

156

This looks like it's actually an "FI" format instruction. I suggest the following:

def FENCE : FI<0b000, 0b0001111, (outs), (ins uimm4:$pred, uimm4:$succ), "fence\t$pred, $succ", []>
{   
  bits<4> pred;
  bits<4> succ;  
  
  let rs1 = 0;
  let rd = 0;  
  let imm12 = {0b0000,pred,succ};  
}

168

def FENCEI : FI<0b001, 0b0001111, (outs), (ins), "fence.i", []> {
  let rs1 = 0;
  let rd = 0;
  let imm12 = 0;
}

190

Missing the csrr and csrw aliases.

test/MC/RISCV/rv32i-valid.s

118

This can be supported easily via adding:

def : InstAlias<"fence", (FENCE 0, 15)>;

(That also makes disassembly of "fence 0, 15" show up as "fence", automatically.

[asb]

Refresh patch and incorporate suggestion from @jyknight regarding FENCE and FENCEI (thanks!).

I _think_ the discussion about naming immediate types was resolved with the use of simm13_lsb0, but let me know if there are still concerns. Using semantic names like branchimm or similar isn't ideal as the names may not hold for further RISC-V extensions (out-of-tree custom extensions or future standard extensions). imm_frm_r, imm_frm_i or similar could be an option, but I'm not really seeing a strong advantage. Input welcome though.

lib/Target/RISCV/RISCVInstrInfo.td

190

I'm intentionally missing aliases in this patch. I'd rather introduce them all together later.

[asb]

The diff I attached a few hours ago didn't include all context, this update fixes that. Sorry for the noise.

[Florob]

test/MC/RISCV/rv32i-valid.s

118

Upstream GAS also requires the arguments to be a substring of iorw and apparently doesn't accept integers.

[asb]

Thanks to @Florob for noting that gas doesn't accept integer arguments to fence. I've updating this patch so that 'iorw' are accepted under the same conditions as gas (no repeated letters, must be given in that order).

I believe this patch is ready for merging.

[asb]

I should have said - please do take a look at the handling of the fence arguments in RISCVAsmParser. I've actually avoided adding a new RISCVOperand type or directly modifying the operand parsing machinery (as AArch64 does for CondCodes). Allowing whatever is there to be parsed, then working out if it's valid or not seemed to more in line with the rest of the MC assembler parser.

[asb]

Ping?

[apazos]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

259

Another code standard note: be consistent where you put the default case, people usually put it as the first case to avoid forgetting it.

413

code standard reminder: {} are unnecessary with one line statement.

[asb]

Address comments from @apazos (thanks!). I've also converted a few if conditions to use MCAsmLexer::{is,isNot}.

[asb]

Ping?

[theraven]

I think that it's probably about time to move the RISC-V back end code to post-commit review.

[asb]

I think a number of future RISC-V backend patches will be straight-forward enough to just use post-commit review. However, the developer policy specifically warns against abandoning the review process and committing directly once a patch has been submitted https://llvm.org/docs/DeveloperPolicy.html#code-reviews

[mgrang]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

149

Shouldn't this be:

for (char &c : Str)

Refer: https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

253

Ditto.

[theraven]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

149

Do you really think that making a copy of a char and reusing it in a register is more expensive than taking a reference to a char in the middle of the string and relying on alias analysis to ensure that we only load via that pointer once?

References to small (register or pair of register) POD values are likely to have more overhead in a range-based for loop than copies and should only be used if you need to modify the value in place.

[asb]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

149

Thanks for the feedback Mandeep. I think using a reference is unnecessary here for the same reason you typically wouldn't use a reference when declaring a function taking an int or char argument. Making c const would perhaps be a better incremental improvement, but given c's short scope it wouldn't add much to readability. [I don't think LLVM has a consistent policy on declaring local PODs as const, but could be wrong]

[reames]

LGTM w/comments applied before commit.

Note: I'm LGTM this after looking for mostly stylistic issues. I did not closely review the ISA specification to confirm the RISCV specific instruction details. I'm mostly LGTMing this because it's been stuck in review for a while, I want to get it unblocked, and I don't see any obvious reasons to hold it back.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

138

/// comment describing function

139

I think you're missing an isImm check here?

150

really minor: a switch would be more clear

152

Should this check be inverted for an ascending order?

249

Just use a cast<> and drop the separate assert.

411–415

Better to invert this and make the error the early return.

lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

26 ↗

(On Diff #112747)

Is there a need to be particularly short here? If not, something like InstFormatR might be more clear.

[asb]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

139

It shouldn't be necessary, but yes - let's add it in case things change in the future. Thanks.

150

Do you find this clearer? It seems slightly less clear to me, but obviously these things are very subjective.

for (char c : Str) {
   if (c <= Prev)
      return false;
  switch (c) {
  default:
    return false;
  case 'i':
  case 'o':
  case 'r':
  case 'w':
    Prev = c;
  }
}

152

'iorw' is accepted, but 'wroi' would not be, matching the GCC behaviour. Reading the *AsmParser.cpp files is made somewhat confusing by the fact methods like ParseInstruction use false for success, unlike these predicates (which are called by tablegenned code).

411–415

I played around with this, and think early-exit for success reads more clearly, particularly as I want to consistently early exit on the same condition (e.g. a couple of lines above we also early-exist on success). There are many more possible incorrect inputs than correct ones, so filtering out the correct ones and having a catch-all for failures at the end makes more sense to me. Happy to change if you feel strongly otherwise.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

 using namespace llvm;
 
 namespace {
 struct RISCVOperand;
 
 class RISCVAsmParser : public MCTargetAsmParser {
   SMLoc getLoc() const { return getParser().getTok().getLoc(); }
 
+  bool generateImmOutOfRangeError(OperandVector &Operands, uint64_t ErrorInfo,
+                                  int Lower, int Upper, Twine Msg);
+
   bool MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
                                OperandVector &Operands, MCStreamer &Out,
                                uint64_t &ErrorInfo,
                                bool MatchingInlineAsm) override;
 
   bool ParseRegister(unsigned &RegNo, SMLoc &StartLoc, SMLoc &EndLoc) override;
 
   bool ParseInstruction(ParseInstructionInfo &Info, StringRef Name,
                         SMLoc NameLoc, OperandVector &Operands) override;
 
   bool ParseDirective(AsmToken DirectiveID) override;
 
 // Auto-generated instruction matching functions
 #define GET_ASSEMBLER_HEADER
 #include "RISCVGenAsmMatcher.inc"
 
   OperandMatchResultTy parseImmediate(OperandVector &Operands);
   OperandMatchResultTy parseRegister(OperandVector &Operands);
+  OperandMatchResultTy parseMemOpBaseReg(OperandVector &Operands);
 
   bool parseOperand(OperandVector &Operands);
 
 public:
   enum RISCVMatchResultTy {
     Match_Dummy = FIRST_TARGET_MATCH_RESULT_TY,
 #define GET_OPERAND_DIAGNOSTIC_TYPES
 #include "RISCVGenAsmMatcher.inc"
bool isConstantImm() const {
     return isImm() && dyn_cast<MCConstantExpr>(getImm());
   }
 
   int64_t getConstantImm() const {
     const MCExpr *Val = getImm();
     return static_cast<const MCConstantExpr *>(Val)->getValue();
   }
 
+  // Predicate methods for AsmOperands defined in RISCVInstrInfo.td
+
+  bool isUImm4() const {
+    return (isConstantImm() && isUInt<4>(getConstantImm()));
+  }
+
+  bool isUImm5() const {
+    return (isConstantImm() && isUInt<5>(getConstantImm()));
+  }
+
   bool isSImm12() const {
     return (isConstantImm() && isInt<12>(getConstantImm()));
   }
 
+  bool isUImm12() const {
+    return (isConstantImm() && isUInt<12>(getConstantImm()));
+  }
+
+  bool isSImm13Lsb0() const {
+    return (isConstantImm() && isShiftedInt<12, 1>(getConstantImm()));
+  }
+
+  bool isUImm20() const {
+    return (isConstantImm() && isUInt<20>(getConstantImm()));
+  }
+
+  bool isSImm21Lsb0() const {
+    return (isConstantImm() && isShiftedInt<20, 1>(getConstantImm()));
+  }
+
   /// getStartLoc - Gets location of the first token of this operand
   SMLoc getStartLoc() const override { return StartLoc; }
   /// getEndLoc - Gets location of the last token of this operand
   SMLoc getEndLoc() const override { return EndLoc; }
 
   unsigned getReg() const override {
     assert(Kind == Register && "Invalid type access!");
     return Reg.RegNum;
   }
 
public:
 
   void addImmOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     addExpr(Inst, getImm());
   }
 };
 } // end anonymous namespace.
 
 #define GET_REGISTER_MATCHER
 #define GET_MATCHER_IMPLEMENTATION
 #define GET_SUBTARGET_FEATURE_NAME
 #include "RISCVGenAsmMatcher.inc"
 
+bool RISCVAsmParser::generateImmOutOfRangeError(
+    OperandVector &Operands, uint64_t ErrorInfo, int Lower, int Upper,
+    Twine Msg = "immediate must be an integer in the range") {
+  SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+  return Error(ErrorLoc, Msg + " [" + Twine(Lower) + ", " + Twine(Upper) + "]");
+}
+
 bool RISCVAsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
                                              OperandVector &Operands,
                                              MCStreamer &Out,
                                              uint64_t &ErrorInfo,
                                              bool MatchingInlineAsm) {
   MCInst Inst;
-  SMLoc ErrorLoc;
 
   switch (MatchInstructionImpl(Operands, Inst, ErrorInfo, MatchingInlineAsm)) {
   default:
     break;
   case Match_Success:
     Inst.setLoc(IDLoc);
     Out.EmitInstruction(Inst, getSTI());
     return false;
   case Match_MissingFeature:
     return Error(IDLoc, "instruction use requires an option to be enabled");
   case Match_MnemonicFail:
     return Error(IDLoc, "unrecognized instruction mnemonic");
-  case Match_InvalidOperand:
-    ErrorLoc = IDLoc;
+  case Match_InvalidOperand: {
+    SMLoc ErrorLoc = IDLoc;
     if (ErrorInfo != ~0U) {
       if (ErrorInfo >= Operands.size())
         return Error(ErrorLoc, "too few operands for instruction");
 
       ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
       if (ErrorLoc == SMLoc())
         ErrorLoc = IDLoc;
     }
     return Error(ErrorLoc, "invalid operand for instruction");
+  }
+  case Match_InvalidUImm4:
+    return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 4) - 1);
+  case Match_InvalidUImm5:
+    return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 5) - 1);
   case Match_InvalidSImm12:
-    SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
-    return Error(ErrorLoc,
-                 "immediate must be an integer in the range [-2048, 2047]");
+    return generateImmOutOfRangeError(Operands, ErrorInfo, -(1 << 11),
+                                      (1 << 11) - 1);
+  case Match_InvalidUImm12:
+    return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 12) - 1);
+  case Match_InvalidSImm13Lsb0:
+    return generateImmOutOfRangeError(
+        Operands, ErrorInfo, -(1 << 12), (1 << 12) - 2,
+        "immediate must be a multiple of 2 bytes in the range");
+  case Match_InvalidUImm20:
+    return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 20) - 1);
+  case Match_InvalidSImm21Lsb0:
+    return generateImmOutOfRangeError(
+        Operands, ErrorInfo, -(1 << 20), (1 << 20) - 2,
+        "immediate must be a multiple of 2 bytes in the range");
   }
 
   llvm_unreachable("Unknown match type detected!");
 }
 
 bool RISCVAsmParser::ParseRegister(unsigned &RegNo, SMLoc &StartLoc,
                                    SMLoc &EndLoc) {
   const AsmToken &Tok = getParser().getTok();
RISCVAsmParser::parseImmediate(OperandVector &Operands) {
   if (getParser().parseExpression(IdVal))
     return MatchOperand_ParseFail;
 
   SMLoc E = SMLoc::getFromPointer(S.getPointer() - 1);
   Operands.push_back(RISCVOperand::createImm(IdVal, S, E));
   return MatchOperand_Success;
 }
 
+RISCVAsmParser::OperandMatchResultTy
+RISCVAsmParser::parseMemOpBaseReg(OperandVector &Operands) {
+  if (getLexer().getKind() != AsmToken::LParen) {
+    Error(getLoc(), "expected '('");
+    return MatchOperand_ParseFail;
+  }
+
+  getParser().Lex(); // Eat '('
+  Operands.push_back(RISCVOperand::createToken("(", getLoc()));
+
+  if (parseRegister(Operands) != MatchOperand_Success) {
+    Error(getLoc(), "expected register");
+    return MatchOperand_ParseFail;
+  }
+
+  if (getLexer().getKind() != AsmToken::RParen) {
+    Error(getLoc(), "expected ')'");
+    return MatchOperand_ParseFail;
+  }
+
+  getParser().Lex(); // Eat ')'
+  Operands.push_back(RISCVOperand::createToken(")", getLoc()));
+
+  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) {
   // Attempt to parse token as register
   if (parseRegister(Operands) == MatchOperand_Success)
     return false;
 
   // Attempt to parse token as an immediate
-  if (parseImmediate(Operands) == MatchOperand_Success)
+  if (parseImmediate(Operands) == MatchOperand_Success) {
+    // Parse memory base register if present
+    if (getLexer().getKind() == AsmToken::LParen) {
+      return parseMemOpBaseReg(Operands) != MatchOperand_Success;
+    }
     return false;
+  }
 
   // Finally we have exhausted all options and must declare defeat.
   Error(getLoc(), "unknown operand");
   return true;
 }
 
 bool RISCVAsmParser::ParseInstruction(ParseInstructionInfo &Info,
                                       StringRef Name, SMLoc NameLoc,

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

uint64_t getBinaryCodeForInstr(const MCInst &MI,
                                  SmallVectorImpl<MCFixup> &Fixups,
                                  const MCSubtargetInfo &STI) const;
 
   /// Return binary encoding of operand. If the machine operand requires
   /// relocation, record the relocation and return zero.
   unsigned getMachineOpValue(const MCInst &MI, const MCOperand &MO,
                              SmallVectorImpl<MCFixup> &Fixups,
                              const MCSubtargetInfo &STI) const;
+
+  unsigned getImmOpValueAsr1(const MCInst &MI, unsigned OpNo,
+                             SmallVectorImpl<MCFixup> &Fixups,
+                             const MCSubtargetInfo &STI) const;
 };
 } // end anonymous namespace
 
 MCCodeEmitter *llvm::createRISCVMCCodeEmitter(const MCInstrInfo &MCII,
                                               const MCRegisterInfo &MRI,
                                               MCContext &Ctx) {
   return new RISCVMCCodeEmitter(Ctx);
 }
RISCVMCCodeEmitter::getMachineOpValue(const MCInst &MI, const MCOperand &MO,
 
   if (MO.isImm())
     return static_cast<unsigned>(MO.getImm());
 
   llvm_unreachable("Unhandled expression!");
   return 0;
 }
 
+unsigned
+RISCVMCCodeEmitter::getImmOpValueAsr1(const MCInst &MI, unsigned OpNo,
+                                      SmallVectorImpl<MCFixup> &Fixups,
+                                      const MCSubtargetInfo &STI) const {
+  const MCOperand &MO = MI.getOperand(OpNo);
+
+  if (MO.isImm()) {
+    unsigned Res = MO.getImm();
+    assert((Res & 1) == 0 && "LSB is non-zero");
+    return Res >> 1;
+  }
+
+  llvm_unreachable("Unhandled expression!");
+}
+
 #include "RISCVGenMCCodeEmitter.inc"

lib/Target/RISCV/RISCVInstrInfo.td

 //===-- RISCVInstrInfo.td - Target Description for RISCV ---*- tablegen -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file describes the RISC-V instructions in TableGen format.
 //
 //===----------------------------------------------------------------------===//
 
 include "RISCVInstrFormats.td"
 
-class SImmAsmOperand<int width>
-  : AsmOperandClass {
-  let Name = "SImm" # width;
+class ImmAsmOperand<string prefix, int width, string suffix> : AsmOperandClass {
+  let Name = prefix # "Imm" # width # suffix;
   let RenderMethod = "addImmOperands";
   let DiagnosticType = !strconcat("Invalid", Name);
 }
 
+class SImmAsmOperand<int width, string suffix = "">
+  : ImmAsmOperand<"S", width, suffix> {
+}
+
+class UImmAsmOperand<int width, string suffix = "">
+  : ImmAsmOperand<"U", width, suffix> {
+}
+
+def uimm4 : Operand<i32> {
+  let ParserMatchClass = UImmAsmOperand<4>;
+}
+
+def uimm5 : Operand<i32> {
+  let ParserMatchClass = UImmAsmOperand<5>;
+}
+
 def simm12 : Operand<i32> {
   let ParserMatchClass = SImmAsmOperand<12>;
 }
 
+def uimm12 : Operand<i32> {
+  let ParserMatchClass = UImmAsmOperand<12>;
+}
+
+// A 13-bit signed immediate where the least significant bit is zero.
+def simm13_lsb0 : Operand<i32> {
+  let ParserMatchClass = SImmAsmOperand<13, "Lsb0">;
+  let EncoderMethod = "getImmOpValueAsr1";
+}
+
+def uimm20 : Operand<i32> {
+  let ParserMatchClass = UImmAsmOperand<20>;
+}
+
+// A 21-bit signed immediate where the least significant bit is zero.
+def simm21_lsb0 : Operand<i32> {
+  let ParserMatchClass = SImmAsmOperand<21, "Lsb0">;
+  let EncoderMethod = "getImmOpValueAsr1";
+}
+
 // As noted in RISCVRegisterInfo.td, the hope is that support for
 // variable-sized register classes will mean that instruction definitions do
 // not need to be duplicated for 32-bit and 64-bit register classes. For now
 // we use 'GPR', which is 32-bit. When codegen for both RV32 and RV64 is
 // added, we will need to duplicate instruction definitions unless a proposal
 // like <http://lists.llvm.org/pipermail/llvm-dev/2016-September/105027.html>
 // is adopted.
 
+def LUI : FU<0b0110111, (outs GPR:$rd), (ins uimm20:$imm20),
+             "lui\t$rd, $imm20", []>;
+
+def AUIPC : FU<0b0010111, (outs GPR:$rd), (ins uimm20:$imm20),
+             "auipc\t$rd, $imm20", []>;
+
+def JAL : FUJ<0b1101111, (outs GPR:$rd), (ins simm21_lsb0:$imm20),
+              "jal\t$rd, $imm20", []>;
+
+def JALR : FI<0b000, 0b1100111, (outs GPR:$rd), (ins GPR:$rs1, simm12:$imm12),
+              "jalr\t$rd, $rs1, $imm12", []>;
+
+class Bcc<bits<3> funct3, string OpcodeStr> :
+      FSB<funct3, 0b1100011, (outs), (ins GPR:$rs1, GPR:$rs2, simm13_lsb0:$imm12),
+         OpcodeStr#"\t$rs1, $rs2, $imm12", []> {
+}
+
+def BEQ   : Bcc<0b000, "beq">;
+def BNE   : Bcc<0b001, "bne">;
+def BLT   : Bcc<0b100, "blt">;
+def BGE   : Bcc<0b101, "bge">;
+def BLTU  : Bcc<0b110, "bltu">;
+def BGEU  : Bcc<0b111, "bgeu">;
+
+class LD_ri<bits<3> funct3, string OpcodeStr> :
+      FI<funct3, 0b0000011, (outs GPR:$rd), (ins GPR:$rs1, simm12:$imm12),
+         OpcodeStr#"\t$rd, ${imm12}(${rs1})", []> {
+  let mayLoad = 1;
+}
+
+def LB   : LD_ri<0b000, "lb">;
+def LH   : LD_ri<0b001, "lh">;
+def LW   : LD_ri<0b010, "lw">;
+def LBU  : LD_ri<0b100, "lbu">;
+def LHU  : LD_ri<0b101, "lhu">;
+
+class ST_ri<bits<3> funct3, string OpcodeStr> :
+      FS<funct3, 0b0100011, (outs), (ins GPR:$rs1, GPR:$rs2, simm12:$imm12),
+         OpcodeStr#"\t$rs2, ${imm12}(${rs1})", []> {
+  let mayStore = 1;
+}
+
+def SB  : ST_ri<0b000, "sb">;
+def SH  : ST_ri<0b001, "sh">;
+def SW  : ST_ri<0b010, "sw">;
+
 class ALU_ri<bits<3> funct3, string OpcodeStr> :
       FI<funct3, 0b0010011, (outs GPR:$rd), (ins GPR:$rs1, simm12:$imm12),
          OpcodeStr#"\t$rd, $rs1, $imm12", []>
 {
 }
 
 def ADDI  : ALU_ri<0b000, "addi">;
 def SLTI  : ALU_ri<0b010, "slti">;
 def SLTIU : ALU_ri<0b011, "sltiu">;
 def XORI  : ALU_ri<0b100, "xori">;
 def ORI   : ALU_ri<0b110, "ori">;
 def ANDI  : ALU_ri<0b111, "andi">;
 
+class SHIFT32_ri<bit arithshift, bits<3> funct3, string OpcodeStr> :
+      FI32Shift<arithshift, funct3, 0b0010011, (outs GPR:$rd), (ins GPR:$rs1, uimm5:$shamt),
+         OpcodeStr#"\t$rd, $rs1, $shamt", []>
+{
+}
+
+def SLLI : SHIFT32_ri<0, 0b001, "slli">;
+def SRLI : SHIFT32_ri<0, 0b101, "srli">;
+def SRAI : SHIFT32_ri<1, 0b101, "srai">;
+
 class ALU_rr<bits<7> funct7, bits<3> funct3, string OpcodeStr> :
       FR<funct7, funct3, 0b0110011, (outs GPR:$rd), (ins GPR:$rs1, GPR:$rs2),
          OpcodeStr#"\t$rd, $rs1, $rs2", []>
 {
 }
 
 def ADD  : ALU_rr<0b0000000, 0b000, "add">;
 def SUB  : ALU_rr<0b0100000, 0b000, "sub">;
 def SLL  : ALU_rr<0b0000000, 0b001, "sll">;
 def SLT  : ALU_rr<0b0000000, 0b010, "slt">;
 def SLTU : ALU_rr<0b0000000, 0b011, "sltu">;
 def XOR  : ALU_rr<0b0000000, 0b100, "xor">;
 def SRL  : ALU_rr<0b0000000, 0b101, "srl">;
 def SRA  : ALU_rr<0b0100000, 0b101, "sra">;
 def OR   : ALU_rr<0b0000000, 0b110, "or">;
 def AND  : ALU_rr<0b0000000, 0b111, "and">;
 
+def FENCE : RISCVInst<(outs), (ins uimm4:$pred, uimm4:$succ), "fence\t$pred, $succ", []>
+{
+  bits<4> pred;
+  bits<4> succ;
+
+  let Opcode = 0b0001111;
+  let Inst{19-7} = 0;
+  let Inst{23-20} = succ;
+  let Inst{27-24} = pred;
+  let Inst{31-28} = 0;
+}
+
+def FENCEI : RISCVInst<(outs), (ins), "fence.i", []> {
+  let Opcode = 0b0001111;
+  let Inst{11-7} = 0;
+  let Inst{14-12} = 0b001;
+  let Inst{31-15} = 0;
+}
+
+let rs1=0, rd=0 in {
+  def ECALL  : FI<0b000, 0b1110011, (outs), (ins), "ecall", []> {
+    let imm12=0;
+  }
+  def EBREAK : FI<0b000, 0b1110011, (outs), (ins), "ebreak", []> {
+    let imm12=1;
+  }
+}
+
+class CSR_rr<bits<3> funct3, string OpcodeStr> :
+      FI<funct3, 0b1110011, (outs GPR:$rd), (ins uimm12:$imm12, GPR:$rs1),
+         OpcodeStr#"\t$rd, $imm12, $rs1", []>
+{
+}
+
+def CSRRW : CSR_rr<0b001, "csrrw">;
+def CSRRS : CSR_rr<0b010, "csrrs">;
+def CSRRC : CSR_rr<0b011, "csrrc">;
+
+class CSR_ri<bits<3> funct3, string OpcodeStr> :
+      FI<funct3, 0b1110011, (outs GPR:$rd), (ins uimm12:$imm12, uimm5:$rs1),
+         OpcodeStr#"\t$rd, $imm12, $rs1", []>
+{
+}
+
+def CSRRWI : CSR_ri<0b101, "csrrwi">;
+def CSRRSI : CSR_ri<0b110, "csrrsi">;
+def CSRRCI : CSR_ri<0b111, "csrrci">;

test/MC/RISCV/rv32i-invalid.s

 # RUN: not llvm-mc -triple riscv32 < %s 2>&1 | FileCheck %s
 
 # Out of range immediates
+## simm12
 ori a0, a1, -2049 # CHECK: :[[@LINE]]:13: error: immediate must be an integer in the range [-2048, 2047]
 andi ra, sp, 2048 # CHECK: :[[@LINE]]:14: error: immediate must be an integer in the range [-2048, 2047]
 
+## imm20
+lui a0, -1 # CHECK: :[[@LINE]]:9: error: immediate must be an integer in the range [0, 1048575]
+lui s0, 1048576 # CHECK: :[[@LINE]]:9: error: immediate must be an integer in the range [0, 1048575]
+auipc zero, -0xf # CHECK: :[[@LINE]]:13: error: immediate must be an integer in the range [0, 1048575]
+
+## simm21_mask1
+jal gp, -1048578 # CHECK: :[[@LINE]]:9: error: immediate must be a multiple of 2 bytes in the range [-1048576, 1048574]
+jal gp, -1048577 # CHECK: :[[@LINE]]:9: error: immediate must be a multiple of 2 bytes in the range [-1048576, 1048574]
+jal gp, 1048575 # CHECK: :[[@LINE]]:9: error: immediate must be a multiple of 2 bytes in the range [-1048576, 1048574]
+jal gp, 1048576 # CHECK: :[[@LINE]]:9: error: immediate must be a multiple of 2 bytes in the range [-1048576, 1048574]
+jal gp, 1 # CHECK: :[[@LINE]]:9: error: immediate must be a multiple of 2 bytes in the range [-1048576, 1048574]
+
+## simm13_mask1
+beq t0, t1, -4098 # CHECK: :[[@LINE]]:13: error: immediate must be a multiple of 2 bytes in the range [-4096, 4094]
+bne t0, t1, -4097 # CHECK: :[[@LINE]]:13: error: immediate must be a multiple of 2 bytes in the range [-4096, 4094]
+blt t0, t1, 4095 # CHECK: :[[@LINE]]:13: error: immediate must be a multiple of 2 bytes in the range [-4096, 4094]
+bge t0, t1, 4096 # CHECK: :[[@LINE]]:13: error: immediate must be a multiple of 2 bytes in the range [-4096, 4094]
+bltu t0, t1, 13 # CHECK: :[[@LINE]]:14: error: immediate must be a multiple of 2 bytes in the range [-4096, 4094]
+bgeu t0, t1, -13 # CHECK: :[[@LINE]]:14: error: immediate must be a multiple of 2 bytes in the range [-4096, 4094]
+
+## imm5
+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]
+
+## imm4
+fence -1, 0 # CHECK: :[[@LINE]]:7: error: immediate must be an integer in the range [0, 15]
+fence 0, -1 # CHECK: :[[@LINE]]:10: error: immediate must be an integer in the range [0, 15]
+fence 16, 0 # CHECK: :[[@LINE]]:7: error: immediate must be an integer in the range [0, 15]
+fence 0, 16 # CHECK: :[[@LINE]]:10: error: immediate must be an integer in the range [0, 15]
+
+## imm12
+csrrw a0, -1, a0 # CHECK: :[[@LINE]]:11: error: immediate must be an integer in the range [0, 4095]
+csrrs a0, 4096, a0 # CHECK: :[[@LINE]]:11: error: immediate must be an integer in the range [0, 4095]
+csrrs a0, -0xf, a0 # CHECK: :[[@LINE]]:11: error: immediate must be an integer in the range [0, 4095]
+csrrc a0, 0x1000, a0 # CHECK: :[[@LINE]]:11: error: immediate must be an integer in the range [0, 4095]
+csrrwi a0, -50, 0 # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [0, 4095]
+csrrsi a0, 4097, a0 # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [0, 4095]
+csrrci a0, 0xffff, a0 # CHECK: :[[@LINE]]:12: error: immediate must be an integer in the range [0, 4095]
+
 # Invalid mnemonics
 subs t0, t2, t1 # CHECK: :[[@LINE]]:1: error: unrecognized instruction mnemonic
 nandi t0, zero, 0 # CHECK: :[[@LINE]]:1: error: unrecognized instruction mnemonic
 
 # Invalid register names
 addi foo, sp, 10 # CHECK: :[[@LINE]]:6: error: unknown operand
 slti a10, a2, 0x20 # CHECK: :[[@LINE]]:6: error: unknown operand
 slt x32, s0, s0 # CHECK: :[[@LINE]]:5: error: unknown operand

test/MC/RISCV/rv32i-valid.s

 # RUN: llvm-mc %s -triple=riscv32 -show-encoding | FileCheck %s
 # RUN: llvm-mc %s -triple=riscv64 -show-encoding | FileCheck %s
 # RUN: llvm-mc %s -triple=riscv32 | FileCheck -check-prefix=CHECK-INST %s
 # RUN: llvm-mc %s -triple=riscv64 | FileCheck -check-prefix=CHECK-INST %s
 
+lui a0, 2 # CHECK: encoding: [0x37,0x25,0x00,0x00]
+# CHECK-INST: lui a0, 2
+lui s11, (0x87000000>>12) # CHECK: encoding: [0xb7,0x0d,0x00,0x87]
+# CHECK-INST: lui s11, 552960
+lui t0, 1048575 # CHECK: encoding: [0xb7,0xf2,0xff,0xff]
+# CHECK-INST: lui t0, 1048575
+lui gp, 0 # CHECK: encoding: [0xb7,0x01,0x00,0x00]
+# CHECK-INST: lui gp, 0
+
+auipc a0, 2 # CHECK: encoding: [0x17,0x25,0x00,0x00]
+# CHECK-INST: auipc a0, 2
+auipc s11, (0x87000000>>12) # CHECK: encoding: [0x97,0x0d,0x00,0x87]
+# CHECK-INST: auipc s11, 552960
+auipc t0, 1048575 # CHECK: encoding: [0x97,0xf2,0xff,0xff]
+# CHECK-INST: auipc t0, 1048575
+auipc gp, 0 # CHECK: encoding: [0x97,0x01,0x00,0x00]
+# CHECK-INST: auipc gp, 0
+
+jal a2, 1048574 # CHECK: encoding: [0x6f,0xf6,0xff,0x7f]
+# CHECK-INST: jal a2, 1048574
+jal a3, 256 # CHECK: encoding: [0xef,0x06,0x00,0x10]
+# CHECK-INST: jal a3, 256
+
+jalr a0, a1, -2048 # CHECK: encoding: [0x67,0x85,0x05,0x80]
+# CHECK-INST: jalr a0, a1, -2048
+jalr t2, t1, 2047 # CHECK: encoding: [0xe7,0x03,0xf3,0x7f]
+# CHECK-INST: jalr t2, t1, 2047
+jalr sp, zero, 256 # CHECK: encoding: [0x67,0x01,0x00,0x10]
+# CHECK-INST: jalr sp, zero, 256
+
+beq s1, s1, 102 # CHECK: encoding: [0x63,0x83,0x94,0x06]
+# CHECK-INST: beq s1, s1, 102
+bne a4, a5, -4096 # CHECK: encoding: [0x63,0x10,0xf7,0x80]
+# CHECK-INST: bne a4, a5, -4096
+blt sp, gp, 4094 # CHECK: encoding: [0xe3,0x4f,0x31,0x7e]
+# CHECK-INST: blt sp, gp, 4094
+bge s2, ra, -224 # CHECK: encoding: [0xe3,0x50,0x19,0xf2]
+# CHECK-INST: bge s2, ra, -224
+bltu zero, zero, 0 # CHECK: encoding: [0x63,0x60,0x00,0x00]
+# CHECK-INST: bltu zero, zero, 0
+bgeu s8, sp, 512 # CHECK: encoding: [0x63,0x70,0x2c,0x20]
+# CHECK-INST: bgeu s8, sp, 512
+
+lb s3, 4(ra) # CHECK: encoding: [0x83,0x89,0x40,0x00]
+# CHECK-INST: lb s3, 4(ra)
+lb s3, +4(ra) # CHECK: encoding: [0x83,0x89,0x40,0x00]
+# CHECK-INST: lb s3, 4(ra)
+lh t1, -2048(zero) # CHECK: encoding: [0x03,0x13,0x00,0x80]
+# CHECK-INST: lh t1, -2048(zero)
+lh sp, 2047(a0) # CHECK: encoding: [0x03,0x11,0xf5,0x7f]
+# CHECK-INST: lh sp, 2047(a0)
+lw a0, 97(a2) # CHECK: encoding: [0x03,0x25,0x16,0x06]
+# CHECK-INST: lw a0, 97(a2)
+lbu s5, 0(s6) # CHECK: encoding: [0x83,0x4a,0x0b,0x00]
+# CHECK-INST: lbu s5, 0(s6)
+lhu t3, 255(t3) # CHECK: encoding: [0x03,0x5e,0xfe,0x0f]
+# CHECK-INST: lhu t3, 255(t3)
+
+sb a0, 2047(a2) # CHECK: encoding: [0xa3,0x0f,0xa6,0x7e]
+# CHECK-INST: sb a0, 2047(a2)
+sh t3, -2048(t5) # CHECK: encoding: [0x23,0x10,0xcf,0x81]
+# CHECK-INST: sh t3, -2048(t5)
+sw ra, 999(zero) # CHECK: encoding: [0xa3,0x23,0x10,0x3e]
+# CHECK-INST: sw ra, 999(zero)
+
 addi ra, sp, 2 # CHECK: encoding: [0x93,0x00,0x21,0x00]
 # CHECK-INST: addi ra, sp, 2
 slti a0, a2, -20 # CHECK: encoding: [0x13,0x25,0xc6,0xfe]
 # CHECK-INST: slti a0, a2, -20
 sltiu s2, s3, 0x50 # CHECK: encoding: [0x13,0xb9,0x09,0x05]
 # CHECK-INST: sltiu s2, s3, 80
 xori tp, t1, -99 # CHECK: encoding: [0x13,0x42,0xd3,0xf9]
 # CHECK-INST: xori tp, t1, -99
 ori a0, a1, -2048 # CHECK: encoding: [0x13,0xe5,0x05,0x80]
 # CHECK-INST: ori a0, a1, -2048
 andi ra, sp, 2047 # CHECK: encoding: [0x93,0x70,0xf1,0x7f]
 # CHECK-INST: andi ra, sp, 2047
 andi x1, x2, 2047 # CHECK: encoding: [0x93,0x70,0xf1,0x7f]
 # CHECK-INST: andi ra, sp, 2047
 
+slli t3, t3, 31 # CHECK: encoding: [0x13,0x1e,0xfe,0x01]
+# CHECK-INST: slli t3, t3, 31
+srli a0, a4, 0 # CHECK: encoding: [0x13,0x55,0x07,0x00]
+# CHECK-INST: srli a0, a4, 0
+srai a2, sp, 15 # CHECK: encoding: [0x13,0x56,0xf1,0x40]
+# CHECK-INST: srai a2, sp, 15
+
 add ra, zero, zero # CHECK: encoding: [0xb3,0x00,0x00,0x00]
 # CHECK-INST: add ra, zero, zero
 add x1, x0, x0 # CHECK: encoding: [0xb3,0x00,0x00,0x00]
 # CHECK-INST: add ra, zero, zero
 sub t0, t2, t1 # CHECK: encoding: [0xb3,0x82,0x63,0x40]
 # CHECK-INST: sub t0, t2, t1
 sll a5, a4, a3 # CHECK: encoding: [0xb3,0x17,0xd7,0x00]
 # CHECK-INST: sll a5, a4, a3
 slt s0, s0, s0 # CHECK: encoding: [0x33,0x24,0x84,0x00]
 # CHECK-INST: slt s0, s0, s0
 sltu gp, a0, a1 # CHECK: encoding: [0xb3,0x31,0xb5,0x00]
 # CHECK-INST: sltu gp, a0, a1
 xor s2, s2, s8 # CHECK: encoding: [0x33,0x49,0x89,0x01]
 # CHECK-INST: xor s2, s2, s8
 xor x18, x18, x24 # CHECK: encoding: [0x33,0x49,0x89,0x01]
 # CHECK-INST: xor s2, s2, s8
 srl a0, s0, t0 # CHECK: encoding: [0x33,0x55,0x54,0x00]
 # CHECK-INST: srl a0, s0, t0
 sra t0, s2, zero # CHECK: encoding: [0xb3,0x52,0x09,0x40]
 # CHECK-INST: sra t0, s2, zero
 or s10, t1, ra # CHECK: encoding: [0x33,0x6d,0x13,0x00]
 # CHECK-INST: or s10, t1, ra
 and a0, s2, s3 # CHECK: encoding: [0x33,0x75,0x39,0x01]
 # CHECK-INST: and a0, s2, s3
+
+# TODO: gnu assembler supports fence with no arguments
+fence 0, 15 # CHECK: encoding: [0x0f,0x00,0xf0,0x00]
+# CHECK-INST: fence 0, 15
+fence 15, 0 # CHECK: encoding: [0x0f,0x00,0x00,0x0f]
+# CHECK-INST: fence 15, 0
+fence 4, 9 # CHECK: encoding: [0x0f,0x00,0x90,0x04]
+# CHECK-INST: fence 4, 9
+fence.i # CHECK: encoding: [0x0f,0x10,0x00,0x00]
+# CHECK-INST: fence.i
+
+ecall # CHECK: encoding: [0x73,0x00,0x00,0x00]
+# CHECK-INST: ecall
+ebreak # CHECK: encoding: [0x73,0x00,0x10,0x00]
+# CHECK-INST: ebreak
+
+csrrw t0, 0xfff, t1 # CHECK: encoding: [0xf3,0x12,0xf3,0xff]
+# CHECK-INST: csrrw t0, 4095, t1
+csrrs s0, 0xc00, x0 # CHECK: encoding: [0x73,0x24,0x00,0xc0]
+# CHECK-INST: csrrs s0, 3072, zero
+csrrs s3, 0x001, s5 # CHECK: encoding: [0xf3,0xa9,0x1a,0x00]
+# CHECK-INST: csrrs s3, 1, s5
+csrrc sp, 0x000, ra # CHECK: encoding: [0x73,0xb1,0x00,0x00]
+# CHECK-INST: csrrc sp, 0, ra
+csrrwi a5, 0x000, 0 # CHECK: encoding: [0xf3,0x57,0x00,0x00]
+# CHECK-INST: csrrwi a5, 0, 0
+csrrsi t2, 0xfff, 31 # CHECK: encoding: [0xf3,0xe3,0xff,0xff]
+# CHECK-INST: csrrsi t2, 4095, 31
+csrrci t1, 0x140, 5 # CHECK: encoding: [0x73,0xf3,0x02,0x14]
+# CHECK-INST: csrrci t1, 320, 5