[PATCH] D93798: [CSKY 4/n] Add basic CSKYAsmParser and CSKYInstPrinter

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:78
+
+/// CSKYOperand - Instances of this class represent a parsed machine
+/// instruction
----------------
Delete `CSKYOperand - `
https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
"Don’t duplicate the documentation comment in the header file ..."


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:81
+struct CSKYOperand : public MCParsedAsmOperand {
+
+  enum KindTy {
----------------
Delete the blank line


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:168
+
+  /// getStartLoc - Gets location of the first token of this operand
+  SMLoc getStartLoc() const override { return StartLoc; }
----------------
"Don’t duplicate the documentation comment in the header file ..."


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:170
+  SMLoc getStartLoc() const override { return StartLoc; }
+  /// getEndLoc - Gets location of the last token of this operand
+  SMLoc getEndLoc() const override { return EndLoc; }
----------------
"Don’t duplicate the documentation comment in the header file ..."


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:194
+    case Register:
+      OS << "<register x";
+      OS << getReg() << ">";
----------------
can use one `OS`


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:237
+
+  // Used by the TableGen Code
+  void addRegOperands(MCInst &Inst, unsigned N) const {
----------------
Complete sentences end with periods.

This applies to many other places.


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:289
+      if (MissingFeatures[i]) {
+        Msg += FirstFeature ? " " : ", ";
+        Msg += getSubtargetFeatureName(i);
----------------
May use D94649


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:434
+                                     SMLoc NameLoc, OperandVector &Operands) {
+  // First operand is token for instruction
+  Operands.push_back(CSKYOperand::createToken(Name, NameLoc));
----------------
Period.


================
Comment at: llvm/lib/Target/CSKY/MCTargetDesc/CSKYInstPrinter.cpp:102
+}
\ No newline at end of file

----------------
No newline at end of file


================
Comment at: llvm/lib/Target/CSKY/MCTargetDesc/CSKYMCTargetDesc.cpp:72
+    CPUName = "generic";
+  return createCSKYMCSubtargetInfoImpl(TT, CPUName, /*TuneCPU*/ CPUName, FS);
+}
----------------
clang-format prefers `/*TuneCPU=*/`


================
Comment at: llvm/test/MC/CSKY/csky-invalid.s:1
+# RUN: not llvm-mc -triple csky < %s 2>&1 | FileCheck %s
+
----------------
Instead of a separate error test, consider merging into the positive test and use `.ifdef ERR` (you can find several examples in test/MC added by me)


================
Comment at: llvm/test/MC/CSKY/csky-invalid.s:4
+## uimm12/oimm12/nimm12
+addi32 a0, a0, 0 # CHECK: :[[@LINE]]:16: error: immediate must be an integer in the range [1, 4096]
+subi32 a0, a0, 4097 # CHECK: :[[@LINE]]:16: error: immediate must be an integer in the range [1, 4096]
----------------
`[[@LINE]]` is deprecated lit syntax. Use `[[#@LINE]]`


================
Comment at: llvm/test/MC/CSKY/csky-valid.s:1
+# RUN: llvm-mc %s -triple=csky -show-encoding \
+# RUN:     | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
----------------
Instead of `csky-valid.s`, you may pick `basic.s`. every file in this directory is C-SKY related so the name `csky.s` does not convey additional information. `-valid` can be omitted because most tests are positive tests.


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

https://reviews.llvm.org/D93798