[RISCV 6/10] Add basic RISCVAsmParser

[asb]

Summary

This doesn't yet support parsing things like %pcrel_hi(foo), but will handle basic instructions with register or immediate operands.

Note: this patch is reliant on D23496

[t.p.northover]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

298 ↗

(On Diff #68189)

I think this should be MatchOperand_NoMatch. ParseFail normally means you've already lexed something so backtracking is impossible.

[reames]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

175 ↗

(On Diff #68189)

style: shouldn't this be createToken?

203 ↗

(On Diff #68189)

This seems odd. Is this a standard convention? Or can we lift this check into the caller and assert the Expr is not null?

346 ↗

(On Diff #68189)

This is a reasonable amount of parsing code without any associated tests. Can we write even trivial tests for this code at this point?

[asb]

Address review comments. Note that the way we defined AsmOperandClass has changed, which will be built upon in the next patch.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

203 ↗

(On Diff #68189)

Some targets define a whole bunch of AsmOperandClass, which means they'll have a lot of addFooOperands which eventually call addExpr. Checking !Expr in addExpr seems to be what the other backends I've checked do. That said, none of my testcases result in a null Expr so it may be one of those things that has been copied blindly by every backend author. I've added an assert to addExpr.

346 ↗

(On Diff #68189)

The next patch (D23564) adds tests and not much else (a very trivial instruction printer).

[t.p.northover]

Just spotted one nit on my last pass through the code:

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

360–361 ↗

(On Diff #69352)

Isn't the first check redundant here?

[jyknight]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

30–33 ↗

(On Diff #69352)

Why are these here? Looks like MCTargetAsmParser already has a Parser member and these accessors.

34 ↗

(On Diff #69352)

Similarly, already stored in MCTargetAsmParser, accessible via getSTI().

[asb]

Address review comments from @jyknight and @t.p.northover. Also use # rather than !strconcat in RISCVInstrInfo.td

[asb]

Refresh to follow changes made in rL283702, putting TheRISCV32Target and TheRISCV64Target behind accessors. Additionally, drop the unused getEndLoc after rL283691 removed it from MCParsedAsmOperand

[grosser]

Hi Alex,

I silently followed the RISC-V patches being upstreamed and did not see an update here for a little while. Is this patch blocked on something or did you just not get to this one yet?

Best,
Tobias

[majnemer]

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

78–81 ↗

(On Diff #74225)

Isn't this just a StringRef?

[asb]

In D23563#639241, @grosser wrote:

Hi Alex,

I silently followed the RISC-V patches being upstreamed and did not see an update here for a little while. Is this patch blocked on something or did you just not get to this one yet?

Due to other responsibilities and my ongoing attempts to find further funding I'm a little behind where I wanted to be with the next set of patches. There were some concerns that aspects of this patchset might need adjusting once codegen was added, which is why I've held back on merging this part of the patchset. In retrospect, this probably wasn't necessary.

[asb]

Update to address review comment (just use StringRef rather than introducing Token struct).

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

78–81 ↗

(On Diff #74225)

Yes, I've replaced it with a StringRef now - thanks.

[alekseyshl]

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/3300 is still unhappy about RISCVAsmParser.cpp

[asb]

@alekseyshl: I've just committed rL310375 which should fix it.

[asb]

RISCV should never have been included in LLVM_ALL_TARGETS when first committed - that was a mistake in the initial 'stub backend' patch. Removing RISCV from LLVM_ALL_TARGETS is the correct thing to do and shouldn't affect end-users, but I'll write to llvm-dev about this tomorrow for wider feedback.

llvm/trunk/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)
 
 add_public_tablegen_target(RISCVCommonTableGen)
 
 add_llvm_target(RISCVCodeGen
   RISCVTargetMachine.cpp
   )
 
+add_subdirectory(AsmParser)
 add_subdirectory(TargetInfo)
 add_subdirectory(MCTargetDesc)

llvm/trunk/lib/Target/RISCV/LLVMBuild.txt

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

llvm/trunk/lib/Target/RISCV/RISCV.td

 
 def Feature64Bit   : SubtargetFeature<"64bit", "HasRV64", "true",
                                       "Implements RV64">;
 
 def : ProcessorModel<"generic-rv32", NoSchedModel, []>;
 
 def : ProcessorModel<"generic-rv64", NoSchedModel, [Feature64Bit]>;
 
+def RISCVAsmParser : AsmParser {
+  let ShouldEmitMatchRegisterAltName = 1;
+}
+
 def RISCV : Target {
   let InstructionSet = RISCVInstrInfo;
+  let AssemblyParsers = [RISCVAsmParser];
 }

llvm/trunk/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"
 
-def simm12 : Operand<i32>;
+class SImmAsmOperand<int width>
+  : AsmOperandClass {
+  let Name = "SImm" # width;
+  let RenderMethod = "addImmOperands";
+  let DiagnosticType = !strconcat("Invalid", Name);
+}
+
+def simm12 : Operand<i32> {
+  let ParserMatchClass = SImmAsmOperand<12>;
+}
 
 // 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.