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

[Lu Weining via Phabricator via llvm-commits]

SixWeining added a comment.

Reply comments inline. I will update this revision soon.



================
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> {
----------------
xen0n wrote:
> As discussed in D115862, people should reach consensus on instruction format naming before continuing with coding.
Right. But this patch is parent of D115862 but not child.

Do other people have any comments for the instruction format naming? @rengolin @MaskRay 


================
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]>;
----------------
xen0n wrote:
> 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?
Yes, v0/v1 are deprecated. I will remove them. Thanks.


================
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]>;
----------------
xen0n wrote:
> Is the "r21" alias needed, given it's identical to its canonical name?
Right. This alias is unnecessary and I will remove it although other targets have similar usage, like the `CSKY`:

```
 78   def R26 : CSKYReg<26, "r26", ["r26"]>, DwarfRegNum<[26]>;
 79   def R27 : CSKYReg<27, "r27", ["r27"]>, DwarfRegNum<[27]>;
```


================
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]>;
----------------
xen0n wrote:
> Same here for `fv0/fv1`.
OK. I will remove them.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchSubtarget.cpp:31
+  if (CPU.empty())
+    CPU = Is64Bit ? "la464" : "generic-la32";
+
----------------
xen0n wrote:
> Nit: why not something like `generic-la64` to keep symmetry?
`la464` is the name of a real CPU core designed by Loongson while `generic-la32` is a dummy CPU name.

Currently we only defined the `la464` ProcessorModel in `LoongArch.td`:
```
94 def : ProcessorModel<"la464", NoSchedModel, [Feature64Bit]>;
```

Maybe we shoulde define another 2 ProcessMode: `generic-la64` and `generic-la32`? And change

```
CPU = Is64Bit ? "la464" : "generic-la32";
```
to

```
CPU = Is64Bit ? "generic-la64" : "generic-la32";
```


================
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;
----------------
xen0n wrote:
> Typo: "determine"
Yes. Thanks.


================
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)
----------------
MaskRay wrote:
> xen0n wrote:
> > Cannot figure this out even as Chinglish... Maybe you meant something like "Check for byte count not multiple of instruction word size"?
> And avoid unneeded parens: `if (Count % 4 != 0)`
Thanks. I will change that.


================
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)
----------------
SixWeining wrote:
> MaskRay wrote:
> > xen0n wrote:
> > > Cannot figure this out even as Chinglish... Maybe you meant something like "Check for byte count not multiple of instruction word size"?
> > And avoid unneeded parens: `if (Count % 4 != 0)`
> Thanks. I will change that.
OK. Thanks.


================
Comment at: llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchMCAsmInfo.cpp:24
+  CodePointerSize = CalleeSaveStackSlotSize = TT.isArch64Bit() ? 8 : 4;
+  // clang-format off
+  AlignmentIsInBytes          = false;
----------------
MaskRay wrote:
> It's unnecessary to align `=`. Just remove alignment and `// clang-format`
OK. I will change that. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115861