[PATCH] D111701: [CSKY] Complete to add basic integer instruction set

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

I have a few comments, but otherwise, it's looking good. Thanks!



================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:98
 /// Instances of this class represent a parsed machine instruction.
+/// instruction
 struct CSKYOperand : public MCParsedAsmOperand {
----------------
extra comment?


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:124
+    unsigned RegNumFrom;
+    unsigned RegNum;
+  };
----------------
>From To? To match the ones below?


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:222
 
+  bool isUImm1() const { return isUImm<1>(); }
   bool isUImm2() const { return isUImm<2>(); }
----------------
This is getting too long, why can't you just use the template version directly?


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:283
 
-  bool isConstpoolSymbol() const {
+  template <unsigned MIN, unsigned MAX> bool isRegSeqTemplate() const {
+    if (!isRegisterSeq())
----------------
Do you really need this templated version?


================
Comment at: llvm/lib/Target/CSKY/AsmParser/CSKYAsmParser.cpp:399
+      else
+        return "noreg";
+    };
----------------
Shouldn't this be an error? Or an assert?


================
Comment at: llvm/lib/Target/CSKY/CSKYInstrInfo.td:312
   def ORI32 : I_16_ZX<"ori32", uimm16,
-  [(set GPR:$rz, (or GPR:$rx, uimm16:$imm16))]>;
+  [(set i32:$rz, (or i32:$rx, uimm16:$imm16))]>;
   def XORI32 : I_12<0x4, "xori32", xor, uimm12>;
----------------
Sorry, I'm missing the context... Why isn't this a GPR? Is `i32` a 32-bit view of the register (like AArch64)?

If so, then `i32` is confusing because of the IR types. Perhaps `GPR32`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111701