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

[wangei via Phabricator via llvm-commits]

wangleiat added inline comments.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:51
+    break;
+  case LoongArchISD::RET:
+    return "LoongArchISD::RET";
----------------
MaskRay wrote:
> Consider using X macro here to avoid future boilerplate.
Good idea. Thanks, i will change that.


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


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:102
+
+  for (unsigned i = 0; i != NumArgs; i++) {
+    MVT ArgVT = Outs[i].VT;
----------------
MaskRay wrote:
> Coding standard uses pre-increments.
OK, thanks. It seems that i have to learn coding standards first. :)


================
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());
----------------
MaskRay wrote:
> Is 16 necessary?
> 
> Suggest `SmallVector<CCValAssign> ArgLocs;`.
OK. I will change that.


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


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


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.h:65
+  /// calling conventions.
+  typedef bool LoongArchCCAssignFn(unsigned ValNo, MVT ValVT,
+                                   CCValAssign::LocInfo LocInfo,
----------------
MaskRay wrote:
> Prefer using to typedef.
> 
> Does llvm/CodeGen/CallingConvLower.h `CCAssignFn` work for you or you just want to remove the `ISD::ArgFlagsTy` argument?
To define a target-specific function, the main reason is that more information needs to be passed to. For example, the abi information to be added in the future. (loongarch calling convention is very similar to risc-v.)


================
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;
----------------
MaskRay wrote:
> In the majority of cases `!` is omitted. https://llvm.org/docs/CodingStandards.html#error-and-warning-messages
OK, thanks.


================
Comment at: llvm/lib/Target/LoongArch/LoongArchInstrInfo.cpp:40
+  llvm_unreachable("LoongArch didn't implement copyPhysReg!");
+  return;
+}
----------------
MaskRay wrote:
> `llvm_unreachable` does need a subsequent return.
Thanks, i will remove it.


================
Comment at: llvm/test/CodeGen/LoongArch/ir-instruction/add.ll:1
+; RUN: llc --mtriple=loongarch32 --verify-machineinstrs < %s \
+; RUN:   | FileCheck %s --check-prefix=CHECK32
----------------
MaskRay wrote:
> --verify-machineinstrs is not necessary.
> 
> LLVM_ENABLE_EXPENSIVE_CHECKS=on builds default to --verify-machineinstrs unless the target sets isMachineVerifierClean to false.
Thanks. I will remove them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122366