[PATCH] D130249: [LoongArch] Improve the calling convention

[Ray Wang via Phabricator via llvm-commits]

wangleiat added a comment.

Thank you very much for your comments and I will revise these.



================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:818
+
+// Eight general-purpose registers a0-a7 used for passing pass integer
+// arguments, with a0-a1 reused to return values. Generally, the GPRs are used
----------------
xen0n wrote:
> duplicate word
thanks


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:826
+// Eight floating-point registers fa0-fa7 used for passing pass floating-point
+// arguments, and fa0-fa1 are also used to return values except soft float ABI.
 const MCPhysReg ArgFPR32s[] = {LoongArch::F0, LoongArch::F1, LoongArch::F2,
----------------
xen0n wrote:
> nit: please rephrase, aren't all FPRs treated as non-existent on soft-float ABIs? Maybe moving this to the front of the sentence is better.
thanks


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:888-889
+
+  // If passing a variadic argument, or if no floating-point argument registers
+  // are available.
+  bool UseGPRForFloat = true;
----------------
xen0n wrote:
> nit: singular form for "no": "no FPR is"
> 
> BTW you can use GPR and FPR across the whole patch to shorten lines. People working with this code are expected to understand such abbreviations (and many more).
thanks


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:899
+  case LoongArchABI::ABI_LP64F:
+    report_fatal_error("Undefined ABI");
+    break;
----------------
xen0n wrote:
> "Unimplemented" would be better?
thanks


================
Comment at: llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp:1011
 
-  // TODO: Handle arguments passed without register.
-  return true;
+  // When a floating-point value is passed on the stack, no bit-conversion is
+  // needed.
----------------
xen0n wrote:
> bit-cast?
thanks again :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130249