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

[Lu Weining via Phabricator via llvm-commits]

SixWeining added a comment.

Thanks for your review @myhsu. I have updated this revision to address your comments.



================
Comment at: llvm/lib/Target/LoongArch/AsmParser/CMakeLists.txt:8
+  LoongArchDesc
+  LoongArchInfo
+  Support
----------------
myhsu wrote:
> nit: lexical ordering
No problem. Thanks.


================
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) {
----------------
myhsu wrote:
> can we use Twine here instead?
I'm afraid not because `Twine` does not override the `+=` operator which is used in the following `for` loop.


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


================
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)
----------------
myhsu wrote:
> why did the diff show disassembler changes here?
Oh, sorry. I forget to move it to D120477. Thanks!


================
Comment at: llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCCodeEmitter.cpp:81
+unsigned
+LoongArchMCCodeEmitter::getImmOpValueSub1(const MCInst &MI, unsigned OpNo,
+                                          SmallVectorImpl<MCFixup> &Fixups,
----------------
myhsu wrote:
> Why did you include code emitter changes here? can you split these (and the `EncoderMethod` changes in the tablegen files) into another patch?
These 2 methods are used by assembler to encode instruction's `unusual` immediate operand.  The word `unusual` means the encoding of an immediate in instruction is not the same as its binary representation. For example,  the `bl` instruction requires a signed 28-bits integer as its operand and the low 2-bits must be 0. So assembler only need to encode the uppper 26-bits into instruction's encoding.

So I think these changes could not be moved otherwise the MC test will fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120476