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

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:389
+  } else {
+    // The second half is passed via the stack, without additional alignment.
+    State.addLoc(CCValAssign::getMem(
----------------
sabuasal wrote:
> 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?
> 
It will. Stack alignment is handled by RISCVFrameLowering::determineFrameLayout and RISCVFrameLowering::emitPrologue. callee_many_scalars and caller_many_scalars in test/CodeGen/RISCV/calling-conv.ll test this code path.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:406
+  assert(LocVT == XLenVT);
+  assert(IsFixed && "Vararg support not yet implemented");
+
----------------
sabuasal wrote:
> nit: don't we use "llvm_unreachable" for yet to be implemented logic?
No, llvm_unreachable is primarily useful for unreachable code (e.g. default statements in a switch). `assert(IsFixed && "Vararg support not yet implemented");` is preferable to:

```
if (IsFixed)
  llvm_unreachable("Vararg support not yet implemented");
```


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:417
+
+  assert(PendingMembers.size() == PendingArgFlags.size());
+
----------------
sabuasal wrote:
> 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?
It would require a refactoring of CCState and likely a modification of other CCState users to move the assert there.


https://reviews.llvm.org/D39898