[PATCH] D115861: [LoongArch 4/6] Add basic tablegen infra for LoongArch

[WÁNG Xuěruì via Phabricator via llvm-commits]

xen0n added inline comments.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrFormats.td:43
+// <opcode | rj | rd>
+class Fmt2R<bits<22> op, dag outs, dag ins, string asmstr,
+            list<dag> pattern = []> : LAInst<outs, ins, asmstr, pattern> {
----------------
As discussed in D115862, people should reach consensus on instruction format naming before continuing with coding.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchRegisterInfo.td:46
+  def R4  : LoongArchReg<4,  "r4", ["a0", "v0"]>, DwarfRegNum<[4]>;
+  def R5  : LoongArchReg<5,  "r5", ["a1", "v1"]>, DwarfRegNum<[5]>;
+  def R6  : LoongArchReg<6,  "r6", ["a2"]>, DwarfRegNum<[6]>;
----------------
According to the [LoongArch ELF psABI doc](https://github.com/loongson/LoongArch-Documentation/blob/main/docs/LoongArch-ELF-ABI-EN.adoc#aliases-for-return-value-registers), we're pushing this code after the `v0/v1` aliases are deprecated, so I think the aliases could simply be removed?


================
Comment at: llvm/lib/Target/LoongArch/LoongArchRegisterInfo.td:62
+  def R20 : LoongArchReg<20, "r20", ["t8"]>, DwarfRegNum<[20]>;
+  def R21 : LoongArchReg<21, "r21", ["r21"]>, DwarfRegNum<[21]>;
+  def R22 : LoongArchReg<22, "r22", ["fp"]>, DwarfRegNum<[22]>;
----------------
Is the "r21" alias needed, given it's identical to its canonical name?


================
Comment at: llvm/lib/Target/LoongArch/LoongArchRegisterInfo.td:102
+  def F0  : LoongArchReg32<0, "f0", ["fa0", "fv0"]>, DwarfRegNum<[32]>;
+  def F1  : LoongArchReg32<1, "f1", ["fa1", "fv1"]>, DwarfRegNum<[33]>;
+  def F2  : LoongArchReg32<2, "f2", ["fa2"]>, DwarfRegNum<[34]>;
----------------
Same here for `fv0/fv1`.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchSubtarget.cpp:31
+  if (CPU.empty())
+    CPU = Is64Bit ? "la464" : "generic-la32";
+
----------------
Nit: why not something like `generic-la64` to keep symmetry?


================
Comment at: llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp:38
+                                                const MCValue &Target) {
+  // TODO: Determin which relocation require special processing at linking time.
+  return false;
----------------
Typo: "determine"


================
Comment at: llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp:44
+                                       const MCSubtargetInfo *STI) const {
+  // Check for a less than instruction size number of bytes
+  if ((Count % 4) != 0)
----------------
Cannot figure this out even as Chinglish... Maybe you meant something like "Check for byte count not multiple of instruction word size"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115861