[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

139

style: shouldn't this be isImm12?

143

Style: sort these by immediate size.

148

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

160

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?

297

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

109

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

139

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.

148

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.

160

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

297

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

149

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.

158

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.

297

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

149

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

149

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

149

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

149

(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

138

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};  
}

150

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

172

Missing the csrr and csrw aliases.

test/MC/RISCV/rv32i-valid.s

66

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

172

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

55

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.

410

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

152

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

152

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

152

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

141

/// comment describing function

142

I think you're missing an isImm check here?

153

really minor: a switch would be more clear

155

Should this check be inverted for an ascending order?

249

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

408–412

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

142

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

153

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;
  }
}

155

'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).

408–412

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

 
 class RISCVAsmParser : public MCTargetAsmParser {
   MCAsmParser &Parser;
 
   MCAsmParser &getParser() const { return Parser; }
   MCAsmLexer &getLexer() const { return Parser.getLexer(); }
   const MCSubtargetInfo &STI;
 
+  SMLoc getLoc() const { return getParser().getTok().getLoc(); }
+
   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();
   }
 
   bool issimm12() const {
     return (isConstantImm() && isInt<12>(getConstantImm()));
   }
 
+  bool isimm20() const {
+    return (isConstantImm() && isUInt<20>(getConstantImm()));
+  }
+
+  bool isimm4() const {
+    return (isConstantImm() && isUInt<4>(getConstantImm()));
+  }
+
+  bool isimm5() const {
+    return (isConstantImm() && isUInt<5>(getConstantImm()));
+  }
+
+  bool issimm21maskb0() const {
+    return (isConstantImm() && isInt<21>(getConstantImm()) &&
+            getConstantImm() % 2 == 0);
+  }
+
+  bool issimm13maskb0() const {
+    return (isConstantImm() && isInt<13>(getConstantImm()) &&
+            getConstantImm() % 2 == 0);
+  }
+
   /// 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::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, STI);
if (ErrorInfo != ~0U) {
         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_Invalidsimm12:
-    SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+    ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
     return Error(ErrorLoc,
                  "immediate must be an integer in the range [-2048, 2047]");
+  case Match_Invalidimm4:
+    ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+    return Error(ErrorLoc, "immediate must be an integer in the range [0, 15]");
+  case Match_Invalidimm5:
+    ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+    return Error(ErrorLoc, "immediate must be an integer in the range [0, 31]");
+  case Match_Invalidimm20:
+    ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+    return Error(ErrorLoc,
+                 "immediate must be an integer in the range [0, 1048575]");
+  case Match_Invalidsimm21maskb0:
+    ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+    return Error(ErrorLoc, "immediate must be a multiple of 2 bytes in the "
+                           "range [-1048576, 1048574]");
+  case Match_Invalidsimm13maskb0:
+    ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+    return Error(
+        ErrorLoc,
+        "immediate must be a multiple of 2 bytes in the range [-4096, 4094]");
   }
 
   llvm_unreachable("Unknown match type detected!");
 }
 
 bool RISCVAsmParser::ParseRegister(unsigned &RegNo, SMLoc &StartLoc,
                                    SMLoc &EndLoc) {
   const AsmToken &Tok = Parser.getTok();
RISCVAsmParser::parseImmediate(OperandVector &Operands) {
   if (getParser().parseExpression(IdVal))
     return MatchOperand_ParseFail;
 
   SMLoc E = SMLoc::getFromPointer(Parser.getTok().getLoc().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;
+  }
+
+  Parser.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;
+  }
+
+  Parser.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(Parser.getTok().getLoc(), "unknown operand");
   return true;
 }
 
 bool RISCVAsmParser::ParseInstruction(ParseInstructionInfo &Info,
                                       StringRef Name, SMLoc NameLoc,

lib/Target/RISCV/RISCVInstrInfo.td

 
 class ImmediateAsmOperand<string name>
   : AsmOperandClass {
   let Name = name;
   let RenderMethod = "addImmOperands";
   let DiagnosticType = !strconcat("Invalid", name);
 }
 
+def imm4 : Operand<i32> {
+  let ParserMatchClass = ImmediateAsmOperand<"imm4">;
+}
+
+def imm5 : Operand<i32> {
+  let ParserMatchClass = ImmediateAsmOperand<"imm5">;
+}
+
 def simm12 : Operand<i32> {
   let ParserMatchClass = ImmediateAsmOperand<"simm12">;
 }
 
+def imm20 : Operand<i32> {
+  let ParserMatchClass = ImmediateAsmOperand<"imm20">;
+}
+
+def simm21maskb0 : Operand<i32> {
+  let ParserMatchClass = ImmediateAsmOperand<"simm21maskb0">;
+}
+
+def simm13maskb0 : Operand<i32> {
+  let ParserMatchClass = ImmediateAsmOperand<"simm13maskb0">;
+}
+
+def LUI : FU<0b0110111, (outs GPR:$rd), (ins imm20:$imm20),
+             "lui\t$rd, $imm20", []>;
+
+
+def AUIPC : FU<0b0010111, (outs GPR:$rd), (ins imm20:$imm20),
+             "auipc\t$rd, $imm20", []>;
+
+def JAL : FUJ<0b1101111, (outs GPR:$rd), (ins simm21maskb0:$imm21),
+              "jal\t$rd, $imm21", []>;
+
+def JALR : FI<0b1100111, 0b000, (outs GPR:$rd), (ins GPR:$rs1, simm12:$imm12),
+              "jalr\t$rd, $rs1, $imm12", []>;
+
+class Bcc<bits<3> funct3, string OpcodeStr> :
+      FSB<0b1100011, funct3, (outs), (ins GPR:$rs1, GPR:$rs2, simm13maskb0:$imm13),
+         !strconcat(OpcodeStr, "\t$rs1, $rs2, $imm13"), []> {
+}
+
+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<0b0000011, funct3, (outs GPR:$rd), (ins GPR:$rs1, simm12:$imm12),
+         !strconcat(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<0b0100011, funct3, (outs), (ins GPR:$rs1, GPR:$rs2, simm12:$imm12),
+         !strconcat(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<0b0010011, funct3, (outs GPR:$rd), (ins GPR:$rs1, simm12:$imm12),
          !strconcat(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">;
 
+// TODO: how to handle difference in the instruction for RV32I and RV64I.
+// Should take an imm6 shamt and check in MatchAndEmitInstruction
+
+class SHIFT32_ri<bits<3> funct3, bit arithshift, string OpcodeStr> :
+      FI32Shift<0b0010011, funct3, arithshift, (outs GPR:$rd), (ins GPR:$rs1, imm5:$shamt),
+         !strconcat(OpcodeStr, "\t$rd, $rs1, $shamt"), []>
+{
+}
+
+def SLLI : SHIFT32_ri<0b001, 0, "slli">;
+def SRLI : SHIFT32_ri<0b101, 0, "srli">;
+def SRAI : SHIFT32_ri<0b101, 1, "srai">;
+
 class ALU_rr<bits<3> funct3, bits<7> funct7, string OpcodeStr> :
       FR<0b0110011, funct3, funct7, (outs GPR:$rd), (ins GPR:$rs1, GPR:$rs2),
          !strconcat(OpcodeStr, "\t$rd, $rs1, $rs2"), []>
 {
 }
 
 def ADD  : ALU_rr<0b000, 0b0000000, "add">;
 def SUB  : ALU_rr<0b000, 0b0100000, "sub">;
 def SLL  : ALU_rr<0b001, 0b0000000, "sll">;
 def SLT  : ALU_rr<0b010, 0b0000000, "slt">;
 def SLTU : ALU_rr<0b011, 0b0000000, "sltu">;
 def XOR  : ALU_rr<0b100, 0b0000000, "xor">;
 def SRL  : ALU_rr<0b101, 0b0000000, "srl">;
 def SRA  : ALU_rr<0b101, 0b0100000, "sra">;
 def OR   : ALU_rr<0b110, 0b0000000, "or">;
 def AND  : ALU_rr<0b111, 0b0000000, "and">;
 
+def FENCE : RISCVInst<(outs), (ins imm4:$pred, imm4:$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 SCALL  : FI<0b1110011, 0b000, (outs), (ins), "scall", []> {
+    let imm12=0;
+  }
+  def SBREAK : FI<0b1110011, 0b000, (outs), (ins), "sbreak", []> {
+    let imm12=1;
+  }
+}
+
+class RD_r<bits<12> csrid, string OpcodeStr> :
+      FI<0b1110011, 0b010, (outs GPR:$rd), (ins),
+         !strconcat(OpcodeStr, "\t$rd"), []>
+{
+  let rs1 = 0;
+  let imm12 = csrid;
+}
+
+def RDCYCLE    : RD_r<0b110000000000, "rdcycle">;
+def RDCYCLEH   : RD_r<0b110010000000, "rdcycleh">;
+def RDTIME     : RD_r<0b110000000001, "rdtime">;
+def RDTIMEH    : RD_r<0b110010000001, "rdtimeh">;
+def RDINSTRET  : RD_r<0b110000000010, "rdinstret">;
+def RDINSTRETH : RD_r<0b110010000010, "rdinstreth">;

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]
+
+## simm21maskb0
+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]
+
+## simm13maskb0
+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]
+
+## 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]
+
 # 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
 
+lui a0, 2 # CHECK: encoding: [0x37,0x25,0x00,0x00]
+lui s11, (0x87000000>>12) # CHECK: encoding: [0xb7,0x0d,0x00,0x87]
+lui t0, 1048575 # CHECK: encoding: [0xb7,0xf2,0xff,0xff]
+lui gp, 0 # CHECK: encoding: [0xb7,0x01,0x00,0x00]
+
+auipc a0, 2 # CHECK: encoding: [0x17,0x25,0x00,0x00]
+auipc s11, (0x87000000>>12) # CHECK: encoding: [0x97,0x0d,0x00,0x87]
+auipc t0, 1048575 # CHECK: encoding: [0x97,0xf2,0xff,0xff]
+auipc gp, 0 # CHECK: encoding: [0x97,0x01,0x00,0x00]
+
+jal a2, 1048574 # CHECK: encoding: [0x6f,0xf6,0xff,0x7f]
+jal a3, 256 # CHECK: encoding: [0xef,0x06,0x00,0x10]
+
+jalr a0, a1, -2048 # CHECK: encoding: [0x67,0x85,0x05,0x80]
+jalr t2, t1, 2047 # CHECK: encoding: [0xe7,0x03,0xf3,0x7f]
+jalr sp, zero, 256 # CHECK: encoding: [0x67,0x01,0x00,0x10]
+
+beq s1, s1, 102 # CHECK: encoding: [0x63,0x83,0x94,0x06]
+bne a4, a5, -4096 # CHECK: encoding: [0x63,0x10,0xf7,0x80]
+blt sp, gp, 4094 # CHECK: encoding: [0xe3,0x4f,0x31,0x7e]
+bge s2, ra, -224 # CHECK: encoding: [0xe3,0x50,0x19,0xf2]
+bltu zero, zero, 0 # CHECK: encoding: [0x63,0x60,0x00,0x00]
+bgeu s8, sp, 512 # CHECK: encoding: [0x63,0x70,0x2c,0x20]
+
+
+lb s3, 4(ra) # CHECK: encoding: [0x83,0x89,0x40,0x00]
+lb s3, +4(ra) # CHECK: encoding: [0x83,0x89,0x40,0x00]
+lh t1, -2048(zero) # CHECK: encoding: [0x03,0x13,0x00,0x80]
+lh sp, 2047(a0) # CHECK: encoding: [0x03,0x11,0xf5,0x7f]
+lw a0, 97(a2) # CHECK: encoding: [0x03,0x25,0x16,0x06]
+lbu s5, 0(s6) # CHECK: encoding: [0x83,0x4a,0x0b,0x00]
+lhu t3, 255(t3) # CHECK: encoding: [0x03,0x5e,0xfe,0x0f]
+
+sb a0, 2047(a2) # CHECK: encoding: [0xa3,0x0f,0xa6,0x7e]
+sh t3, -2048(t5) # CHECK: encoding: [0x23,0x10,0xcf,0x81]
+sw ra, 999(zero) # CHECK: encoding: [0xa3,0x23,0x10,0x3e]
+
 addi ra, sp, 2 # CHECK: encoding: [0x93,0x00,0x21,0x00]
 slti a0, a2, -20 # CHECK: encoding: [0x13,0x25,0xc6,0xfe]
 sltiu s2, s3, 0x50 # CHECK: encoding: [0x13,0xb9,0x09,0x05]
 xori tp, t1, -99 # CHECK: encoding: [0x13,0x42,0xd3,0xf9]
 ori a0, a1, -2048 # CHECK: encoding: [0x13,0xe5,0x05,0x80]
 andi ra, sp, 2047 # CHECK: encoding: [0x93,0x70,0xf1,0x7f]
 andi x1, x2, 2047 # CHECK: encoding: [0x93,0x70,0xf1,0x7f]
 
+slli t3, t3, 31 # CHECK: encoding: [0x13,0x1e,0xfe,0x01]
+srli a0, a4, 0 # CHECK: encoding: [0x13,0x55,0x07,0x00]
+srai a2, sp, 15 # CHECK: encoding: [0x13,0x56,0xf1,0x40]
+
 add ra, zero, zero # CHECK: encoding: [0xb3,0x00,0x00,0x00]
 add x1, x0, x0 # CHECK: encoding: [0xb3,0x00,0x00,0x00]
 sub t0, t2, t1 # CHECK: encoding: [0xb3,0x82,0x63,0x40]
 sll a5, a4, a3 # CHECK: encoding: [0xb3,0x17,0xd7,0x00]
 slt s0, s0, s0 # CHECK: encoding: [0x33,0x24,0x84,0x00]
 sltu gp, a0, a1 # CHECK: encoding: [0xb3,0x31,0xb5,0x00]
 xor s2, s2, s8 # CHECK: encoding: [0x33,0x49,0x89,0x01]
 xor x18, x18, x24 # CHECK: encoding: [0x33,0x49,0x89,0x01]
 srl a0, s0, t0 # CHECK: encoding: [0x33,0x55,0x54,0x00]
 sra t0, s2, zero # CHECK: encoding: [0xb3,0x52,0x09,0x40]
 or s10, t1, ra # CHECK: encoding: [0x33,0x6d,0x13,0x00]
 and a0, s2, s3 # CHECK: encoding: [0x33,0x75,0x39,0x01]
+
+# TODO: gnu assembler supports fence with no arguments
+fence 0, 15 # CHECK: encoding: [0x0f,0x00,0xf0,0x00]
+fence 15, 0 # CHECK: encoding: [0x0f,0x00,0x00,0x0f]
+fence 4, 9 # CHECK: encoding: [0x0f,0x00,0x90,0x04]
+fence.i # CHECK: encoding: [0x0f,0x10,0x00,0x00]
+
+scall # CHECK: encoding: [0x73,0x00,0x00,0x00]
+sbreak # CHECK: encoding: [0x73,0x00,0x10,0x00]
+
+rdcycle s0 # CHECK: encoding: [0x73,0x24,0x00,0xc0]
+rdcycleh s1 # CHECK: encoding: [0xf3,0x24,0x00,0xc8]
+rdtime s2 # CHECK: encoding: [0x73,0x29,0x10,0xc0]
+rdtimeh s3 # CHECK: encoding: [0xf3,0x29,0x10,0xc8]
+rdinstret s4 # CHECK: encoding: [0x73,0x2a,0x20,0xc0]
+rdinstreth s5 # CHECK: encoding: [0xf3,0x2a,0x20,0xc8]