[PATCH] D112919: [CSKY] Add CSKY 16-bit instruction format and encoding

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: llvm/lib/Target/CSKY/CSKY.td:28
 
+def FeatureJAVA
+    : SubtargetFeature<"java", "HasJAVA", "true", "Enable java instructions">;
----------------
Huh! I haven't found any reference to Java instructions in either available manuals (in github).

I'm guessing this is similar to Arm's Jazelle extensions?

None of the instructions below marked with `HasJAVA` appear on the ISA document...

Perhaps better to leave it out for another commit once that part is public?


================
Comment at: llvm/lib/Target/CSKY/CSKYInstrInfo16Instr.td:55
+// Instruction definitions.
+//===----------------------------------------------------------------------===//
+
----------------
It's not necessarily mandatory, but generally useful to have sub-divisions on the TD file itself for instructions of different type.

In the ISA document, the instructions are separated by classes (arithmetic, logic, shift, etc). If they follow the same pattern here, it would be a lot easier to follow (and find) the implementation of each instruction.


================
Comment at: llvm/lib/Target/CSKY/CSKYRegisterInfo.td:156
 
+def sGPR : RegisterClass<"CSKY", [i32], 32,
+                        (add (sequence "R%u", 0, 3), (sequence "R%u", 12, 13), R15,
----------------
Would be nice to have a one-line description for the difference between `GPR`, `sGPR` and `mGPR`, or at least their purpose in instruction use, for example, "only registers that 16-bit instruction can access".

The other existing cases are more obvious as to what they are.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112919