[RISCV 7/10] Add RISCVInstPrinter and basic MC assembler tests

[asb]

Summary

With the addition of RISCVInstPrinter, we can now make use of lit tests.

[reames]

Getting 7 patches in before our first real test seems less than ideal. That's a lot of code to exercise and not many tests doing it. One possibility would be to move this patch a bit earlier in the sequence, and write some c++ code which prints manually constructed instructions. Not sure that's actually worth doing, but I'm intentionally being pedantic as requested. :)

(p.s. I don't work on the backend, so I'm learning as I go through the patches. If something doesn't make sense, that's the probable reason.)

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

11

I believe you mentioned in another thread that the format is not well specified. We should call that out here. What is the goal for this implementation? Compatibility with existing implementation? Implementing some newly defined spec?

24

Should this be riscv-asm-printer? Not sure what convention we use in various backends.

45

reuse printRegName?

[asb]

In D23564#521832, @reames wrote:

Getting 7 patches in before our first real test seems less than ideal. That's a lot of code to exercise and not many tests doing it. One possibility would be to move this patch a bit earlier in the sequence, and write some c++ code which prints manually constructed instructions. Not sure that's actually worth doing, but I'm intentionally being pedantic as requested. :)

I'd say that backend work doesn't start until patch 3, which adds the dummy backend. At that stage there's nothing to test anyway, so really it's patch 4+5+6 that add code that can't yet be tested, then we add the test here. Other than merging this patch into the previous one, I'm not sure what I can do to get the tests started earlier. I'd appreciate suggestions though.

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

11

Basic assembly printing is fine. I think it's more that the accepted pseudo-instructions are under-documented, relocations aren't documented etc. These don't pose an issue for basic printing.

24

I've verified that every other in-tree backend uses DEBUG_TYPE "asm-printer". Was worth checking though, thanks

[asb]

Refresh patch and reuse getRegName as suggested by @reames

[theraven]

LGTM.

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

24

Not much danger of ambiguity - I doubt many people are debugging LLVM with test cases that emit assembly for two different architectures in the same test run...

[t.p.northover]

Shouldn't we be testing the code we're adding here? At the moment we seem to be adding support for InstPrinter but only testing MCTargetDesc (the encodings) & AsmPrinter code.

[jyknight]

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

11

I'd just note that the spec does not document any asm syntax, leaving it up to implementors' imagination. Still, yes, for basic printing, there's not much ambiguity.

test/MC/RISCV/rv32i-valid.s

4

Each of these lines should be like:

# CHECK: addi ra, sp, 2 # encoding: [0x93,0x00,0x21,0x00]
addi ra, sp, 2

So that it is actually checking the output of the the InstPrinter code in addition to the encoding.

[asb]

test/MC/RISCV/rv32i-valid.s

4

I'm about to refresh my patchset, I decided to add a CHECK-INST to separate the checking of assembly printing and encoding. It isn't necessary for this patch, but it's desirable when adding %lo etc:

lui s11, %hi(0x87000000) # CHECK: encoding: [0xb7,0x0d,0x00,0x87]
# CHECK-INST: lui s11, 552960

[asb]

Tests have been modified to check instruction printing.

[jyknight]

test/MC/RISCV/rv32i-valid.s

8

If you flip this order around, putting the "CHECK-INST" first, then the "CHECK: encoding", you wouldn't need to run llvm-mc twice for each processor mode, once with show-encoding and once without; just give FileCheck both prefixes in one run.

I'd also suggest moving the encoding check up to its own line, I think it'll be more readable that way. e.g.:

# RUN: llvm-mc %s -triple=riscv32 -show-encoding | FileCheck -check-prefixes=CHECK,CHECK-INST %s
# RUN: llvm-mc %s -triple=riscv64 -show-encoding | FileCheck -check-prefixes=CHECK,CHECK-INST %s

# CHECK-INST: addi ra, sp, 2
# CHECK: encoding: [0x93,0x00,0x21,0x00]
addi ra, sp, 2

[asb]

Update test style as suggested by @jyknight

test/MC/RISCV/rv32i-valid.s

8

That's definitely nicer - thanks! I've switched to using this style.

lib/Target/RISCV/CMakeLists.txt

 set(LLVM_TARGET_DEFINITIONS RISCV.td)
 
 tablegen(LLVM RISCVGenRegisterInfo.inc -gen-register-info)
 tablegen(LLVM RISCVGenInstrInfo.inc -gen-instr-info)
 tablegen(LLVM RISCVGenMCCodeEmitter.inc -gen-emitter)
 tablegen(LLVM RISCVGenAsmMatcher.inc -gen-asm-matcher)
+tablegen(LLVM RISCVGenAsmWriter.inc -gen-asm-writer)
 
 add_public_tablegen_target(RISCVCommonTableGen)
 
 add_llvm_target(RISCVCodeGen
   RISCVTargetMachine.cpp
   )
 
 add_subdirectory(AsmParser)
-add_subdirectory(TargetInfo)
+add_subdirectory(InstPrinter)
 add_subdirectory(MCTargetDesc)
+add_subdirectory(TargetInfo)

lib/Target/RISCV/InstPrinter/CMakeLists.txt

+add_llvm_library(LLVMRISCVAsmPrinter
+  RISCVInstPrinter.cpp
+  )

lib/Target/RISCV/InstPrinter/LLVMBuild.txt

+;===- ./lib/Target/RISCV/InstPrinter/LLVMBuild.txt -------------*- Conf -*--===;
+;
+;                     The LLVM Compiler Infrastructure
+;
+; This file is distributed under the University of Illinois Open Source
+; License. See LICENSE.TXT for details.
+;
+;===------------------------------------------------------------------------===;
+;
+; This is an LLVMBuild description file for the components in this subdirectory.
+;
+; For more information on the LLVMBuild system, please see:
+;
+;   http://llvm.org/docs/LLVMBuild.html
+;
+;===------------------------------------------------------------------------===;
+
+[component_0]
+type = Library
+name = RISCVAsmPrinter
+parent = RISCV
+required_libraries = MC Support
+add_to_library_groups = RISCV

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.h

+//===-- RISCVInstPrinter.h - Convert RISCV MCInst to asm syntax ---*- C++ -*--//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This class prints a RISCV MCInst to a .s file.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIB_TARGET_RISCV_INSTPRINTER_RISCVINSTPRINTER_H
+#define LLVM_LIB_TARGET_RISCV_INSTPRINTER_RISCVINSTPRINTER_H
+
+#include "MCTargetDesc/RISCVMCTargetDesc.h"
+#include "llvm/MC/MCInstPrinter.h"
+
+namespace llvm {
+class MCOperand;
+
+class RISCVInstPrinter : public MCInstPrinter {
+public:
+  RISCVInstPrinter(const MCAsmInfo &MAI, const MCInstrInfo &MII,
+                   const MCRegisterInfo &MRI)
+      : MCInstPrinter(MAI, MII, MRI) {}
+
+  void printInst(const MCInst *MI, raw_ostream &O, StringRef Annot,
+                 const MCSubtargetInfo &STI) override;
+  void printRegName(raw_ostream &O, unsigned RegNo) const override;
+
+  void printOperand(const MCInst *MI, unsigned OpNo, raw_ostream &O,
+                    const char *Modifier = nullptr);
+
+  // Autogenerated by tblgen.
+  void printInstruction(const MCInst *MI, raw_ostream &O);
+  static const char *getRegisterName(unsigned RegNo,
+                                     unsigned AltIdx = RISCV::ABIRegAltName);
+};
+}
+
+#endif

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp

+//===-- RISCVInstPrinter.cpp - Convert RISCV MCInst to asm syntax ---------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This class prints an RISCV MCInst to a .s file.
+//
+//===----------------------------------------------------------------------===//
+
+#include "RISCVInstPrinter.h"
+#include "llvm/MC/MCAsmInfo.h"
+#include "llvm/MC/MCExpr.h"
+#include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCSymbol.h"
+#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormattedStream.h"
+using namespace llvm;
+
+#define DEBUG_TYPE "asm-printer"
+
+// Include the auto-generated portion of the assembly writer.
+#include "RISCVGenAsmWriter.inc"
+
+void RISCVInstPrinter::printInst(const MCInst *MI, raw_ostream &O,
+                                 StringRef Annot, const MCSubtargetInfo &STI) {
+  printInstruction(MI, O);
+  printAnnotation(O, Annot);
+}
+
+void RISCVInstPrinter::printRegName(raw_ostream &O, unsigned RegNo) const {
+  O << getRegisterName(RegNo);
+}
+
+void RISCVInstPrinter::printOperand(const MCInst *MI, unsigned OpNo,
+                                  raw_ostream &O, const char *Modifier) {
+  assert((Modifier == 0 || Modifier[0] == 0) && "No modifiers supported");
+  const MCOperand &MO = MI->getOperand(OpNo);
+
+  if (MO.isReg()) {
+    printRegName(O, MO.getReg());
+    return;
+  }
+
+  if (MO.isImm()) {
+    O << MO.getImm();
+    return;
+  }
+
+  assert(MO.isExpr() && "Unknown operand kind in printOperand");
+  MO.getExpr()->print(O, &MAI);
+}

lib/Target/RISCV/LLVMBuild.txt

 ;
 ; For more information on the LLVMBuild system, please see:
 ;
 ;   http://llvm.org/docs/LLVMBuild.html
 ;
 ;===------------------------------------------------------------------------===;
 
 [common]
-subdirectories = AsmParser TargetInfo MCTargetDesc
+subdirectories = AsmParser InstPrinter TargetInfo MCTargetDesc
 
 [component_0]
 type = TargetGroup
 name = RISCV
 parent = Target
 has_asmparser = 1
+has_asmprinter = 1
 
 [component_1]
 type = Library
 name = RISCVCodeGen
 parent = RISCV
-required_libraries = Core CodeGen RISCVInfo Support Target
+required_libraries = AsmPrinter Core CodeGen MC RISCVAsmPrinter RISCVDesc RISCVInfo Support Target
 add_to_library_groups = RISCV

lib/Target/RISCV/MCTargetDesc/LLVMBuild.txt

 ;   http://llvm.org/docs/LLVMBuild.html
 ;
 ;===------------------------------------------------------------------------===;
 
 [component_0]
 type = Library
 name = RISCVDesc
 parent = RISCV
-required_libraries = MC RISCVInfo Support
+required_libraries = MC RISCVAsmPrinter RISCVInfo Support
 add_to_library_groups = RISCV

lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp

 //===-- RISCVMCTargetDesc.cpp - RISCV Target Descriptions -----------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// This file provides RISCV-specific target descriptions.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "RISCVMCTargetDesc.h"
 #include "RISCVMCAsmInfo.h"
+#include "InstPrinter/RISCVInstPrinter.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TargetRegistry.h"

 }
 
 static MCAsmInfo *createRISCVMCAsmInfo(const MCRegisterInfo &MRI,
                                        const Triple &TT) {
   MCAsmInfo *MAI = new RISCVMCAsmInfo(TT);
   return MAI;
 }
 
+static MCInstPrinter *createRISCVMCInstPrinter(const Triple &T,
+                                               unsigned SyntaxVariant,
+                                               const MCAsmInfo &MAI,
+                                               const MCInstrInfo &MII,
+                                               const MCRegisterInfo &MRI) {
+  return new RISCVInstPrinter(MAI, MII, MRI);
+}
+
 extern "C" void LLVMInitializeRISCVTargetMC() {
   for (Target *T : {&TheRISCV32Target, &TheRISCV64Target}) {
     RegisterMCAsmInfoFn X(*T, createRISCVMCAsmInfo);
     TargetRegistry::RegisterMCInstrInfo(*T, createRISCVMCInstrInfo);
     TargetRegistry::RegisterMCRegInfo(*T, createRISCVMCRegisterInfo);
     TargetRegistry::RegisterMCAsmBackend(*T, createRISCVAsmBackend);
     TargetRegistry::RegisterMCCodeEmitter(*T, createRISCVMCCodeEmitter);
+    TargetRegistry::RegisterMCInstPrinter(*T, createRISCVMCInstPrinter);
   }
 }

test/MC/RISCV/lit.local.cfg

+if not 'RISCV' in config.root.targets:
+    config.unsupported = True
+

test/MC/RISCV/rv32i-invalid.s

+# RUN: not llvm-mc -triple riscv32 < %s 2>&1 | FileCheck %s
+
+# Out of range immediates
+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]
+
+# 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
+
+# RV64I mnemonics
+addiw a0, sp, 100 # CHECK: :[[@LINE]]:1: error: unrecognized instruction mnemonic
+sraw t0, s2, zero # CHECK: :[[@LINE]]:1: error: unrecognized instruction mnemonic
+
+# Invalid operand types
+xori sp, 22, 220 # CHECK: :[[@LINE]]:10: error: invalid operand for instruction
+sub t0, t2, 1 # CHECK: :[[@LINE]]:13: error: invalid operand for instruction
+
+# Too many operands
+add ra, zero, zero, zero # CHECK: :[[@LINE]]:21: error: invalid operand for instruction
+sltiu s2, s3, 0x50, 0x60 # CHECK: :[[@LINE]]:21: error: invalid operand for instruction
+
+# Too few operands
+ori a0, a1 # CHECK: :[[@LINE]]:1: error: too few operands for instruction
+xor s2, s2 # CHECK: :[[@LINE]]:1: error: too few operands for instruction

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
+
+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]
+
+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]