[PATCH] D122366: [LoongArch] Construct codegen infra and generate first add instruction.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:51
+    break;
+  case LoongArchISD::RET:
+    return "LoongArchISD::RET";
----------------
Consider using X macro here to avoid future boilerplate.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:92
+                        << EVT(ArgVT).getEVTString() << '\n');
+      llvm_unreachable(nullptr);
+    }
----------------
llvm_unreachable takes a string literal. Use `""` instead of nullptr if you don't have a specific message.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:102
+
+  for (unsigned i = 0; i != NumArgs; i++) {
+    MVT ArgVT = Outs[i].VT;
----------------
Coding standard uses pre-increments.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:142
+  // Assign locations to all of the incoming arguments.
+  SmallVector<CCValAssign, 16> ArgLocs;
+  CCState CCInfo(CallConv, IsVarArg, MF, ArgLocs, *DAG.getContext());
----------------
Is 16 necessary?

Suggest `SmallVector<CCValAssign> ArgLocs;`.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:158
+  // directly.
+  if (Outs.size() > 2)
+    return false;
----------------
`return ... <= 2;`


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.h:63
+private:
+  /// LoongArchCCAssignFn - Target-specific function used to lower LoongArch
+  /// calling conventions.
----------------
https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
"Don’t duplicate function or class name at the beginning of the comment."


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.h:65
+  /// calling conventions.
+  typedef bool LoongArchCCAssignFn(unsigned ValNo, MVT ValVT,
+                                   CCValAssign::LocInfo LocInfo,
----------------
Prefer using to typedef.

Does llvm/CodeGen/CallingConvLower.h `CCAssignFn` work for you or you just want to remove the `ISD::ArgFlagsTy` argument?


================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrInfo.cpp:39
+  // TODO: Now, we only support GPR->GPR copies.
+  llvm_unreachable("LoongArch didn't implement copyPhysReg!");
+  return;
----------------
In the majority of cases `!` is omitted. https://llvm.org/docs/CodingStandards.html#error-and-warning-messages


================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrInfo.cpp:40
+  llvm_unreachable("LoongArch didn't implement copyPhysReg!");
+  return;
+}
----------------
`llvm_unreachable` does need a subsequent return.


================
Comment at: llvm/test/CodeGen/LoongArch/ir-instruction/add.ll:1
+; RUN: llc --mtriple=loongarch32 --verify-machineinstrs < %s \
+; RUN:   | FileCheck %s --check-prefix=CHECK32
----------------
--verify-machineinstrs is not necessary.

LLVM_ENABLE_EXPENSIVE_CHECKS=on builds default to --verify-machineinstrs unless the target sets isMachineVerifierClean to false.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122366