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

[Sameer AbuAsal via Phabricator via llvm-commits]

sabuasal added a comment.

Some minor reviews. I will look at the test cases tomorrow to check if we cover most cases.



================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:389
+  } else {
+    // The second half is passed via the stack, without additional alignment.
+    State.addLoc(CCValAssign::getMem(
----------------
So will the stack be aligned upon entry like mentioned here: https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#-integer-calling-convention?



================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:398
+
+static bool CC_RISCV(const DataLayout &DL, unsigned ValNo, MVT ValVT, MVT LocVT,
+                     CCValAssign::LocInfo LocInfo, ISD::ArgFlagsTy ArgFlags,
----------------
function description?


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:406
+  assert(LocVT == XLenVT);
+  assert(IsFixed && "Vararg support not yet implemented");
+
----------------
nit: don't we use "llvm_unreachable" for yet to be implemented logic?


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:417
+
+  assert(PendingMembers.size() == PendingArgFlags.size());
+
----------------
This seems like something CCStat need to worry about. Why are we asserting for this here? plus, shouldn't we add a message describing the assertion if and when it fails?


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:435
+  if (ArgFlags.isSplitEnd() && PendingMembers.size() <= 2) {
+    assert(PendingMembers.size() == 2);
+    // Apply the normal calling convention rules to the first half of the
----------------
assert description?


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:451
+  // If we reach this point and PendingMembers is non-empty, we must be at the
+  // end of a split argument that must be passed indirect.
+  if (!PendingMembers.empty()) {
----------------

> be passed indirect
passed indirectly?


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:453
+  if (!PendingMembers.empty()) {
+    assert(ArgFlags.isSplitEnd());
+    assert(PendingMembers.size() > 2);
----------------
assert description?



https://reviews.llvm.org/D39898