[PATCH] D39898: [RISCV] Add custom CC_RISCV calling convention and improved call support

[Ana Pazos via Phabricator via llvm-commits]

apazos added inline comments.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:342
+// * If a struct could never be passed in registers or directly in a stack
+// slot (as it is larger than 2x xlen and the floating point rules don't
+// apply), then pass it using a pointer with the byval attribute
----------------
capitalize xlen as it is referred as XLEN in other places


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:343
+// slot (as it is larger than 2x xlen and the floating point rules don't
+// apply), then pass it using a pointer with the byval attribute
+// * If a struct is less than 2x xlen, then coerce to either a two-element
----------------
end point missing.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:345
+// * If a struct is less than 2x xlen, then coerce to either a two-element
+// word-sized array or a 2x xlen scalar (depending on alignment).
+// * The frontend can determine whether a struct is returned by reference or
----------------
xlen -> XLEN


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:360
+
+// Pass a 2xlen argument that has been split in to two xlen values through
+// registers or the stack as necessary.
----------------
2xlen -> 2x XLEN? Shound'nt we agree on a convention?


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:386
+    // The second half can also be passed via register.
+    State.addLoc(
+        CCValAssign::getReg(ValNo2, ValVT2, Reg, LocVT2, CCValAssign::Full));
----------------
Maybe if you add Reg1 Reg2 vars upfront, then you can rewrite the logic in a simpler way?

if (Reg1) {
 if (Reg2)
  // both halves in regs
 else
  // first half in reg only, the other on the stack
} else
  // both halves on the stack


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:414
+
+  SmallVectorImpl<CCValAssign> &PendingMembers = State.getPendingLocs();
+  SmallVectorImpl<ISD::ArgFlagsTy> &PendingArgFlags =
----------------
why change the name from PendingLocs to PendingMembers?


https://reviews.llvm.org/D39898