[PATCH] D120476: [LoongArch] Add basic support to AsmParser

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu added inline comments.


================
Comment at: llvm/lib/Target/LoongArch/AsmParser/CMakeLists.txt:8
+  LoongArchDesc
+  LoongArchInfo
+  Support
----------------
nit: lexical ordering


================
Comment at: llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp:395
+    bool FirstFeature = true;
+    std::string Msg = "instruction requires the following:";
+    for (unsigned i = 0, e = MissingFeatures.size(); i != e; ++i) {
----------------
can we use Twine here instead?


================
Comment at: llvm/lib/Target/LoongArch/AsmParser/LoongArchAsmParser.cpp:438
+  case Match_InvalidUImm2:
+    return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 2) - 1);
+  case Match_InvalidUImm2plus1:
----------------
I'm not sure but do you think this will improve readability?


================
Comment at: llvm/lib/Target/LoongArch/CMakeLists.txt:8
 tablegen(LLVM LoongArchGenDAGISel.inc -gen-dag-isel)
+tablegen(LLVM LoongArchGenDisassemblerTables.inc -gen-disassembler)
+tablegen(LLVM LoongArchGenInstrInfo.inc -gen-instr-info)
----------------
why did the diff show disassembler changes here?


================
Comment at: llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp:81
+unsigned
+LoongArchMCCodeEmitter::getImmOpValueSub1(const MCInst &MI, unsigned OpNo,
+                                          SmallVectorImpl<MCFixup> &Fixups,
----------------
Why did you include code emitter changes here? can you split these (and the `EncoderMethod` changes in the tablegen files) into another patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120476/new/

https://reviews.llvm.org/D120476