[PATCH] D60456: [RISCV] Hard float ABI support

[Alex Bradbury via Phabricator via cfe-commits]

asb added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9232
+    if (IsFloat && Size > FLen)
+      return false;
+    // Can't be eligible if an integer type was already found (only fp+int or
----------------
rjmccall wrote:
> Is this the only consideration for floating-point types?  Clang does have increasing support for `half` / various `float16` types.
These types aren't supported on RISC-V currently. As the ABI hasn't really been explicitly confirmed, I've defaulted to the integer ABI in that case. Could move to an assert if you prefer, though obviously any future move to enable half floats for RISC-V should include ABI tests too.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9236
+    if (IsInt && Field1Ty && Field1Ty->isIntegerTy())
+      return false;
+    if (!Field1Ty) {
----------------
rjmccall wrote:
> The comment here is wrong because fp+fp is allowed, right?
> 
> Is this not already caught by the post-processing checks you do in `detectFPCCEligibleStruct`?  Would it make more sense to just do all those checks there?
Thanks, I meant to say int+int isn't eligible. Reworded to say that.

I don't think it would simplify things to do all checks in detectFPCCEligibleStruct. More repetition would be required in order to do checks on both Float1Ty and Float2Ty.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9288
+      return false;
+    for (const FieldDecl *FD : RD->fields()) {
+      const ASTRecordLayout &Layout = getContext().getASTRecordLayout(RD);
----------------
rjmccall wrote:
> I really expect there to be something in this block about whether the field is a bit-field.  What exactly does your ABI specify if there's a bit-field?
I've updated to handle bitfields and submitted a [pull request](https://github.com/riscv/riscv-elf-psabi-doc/pull/100) to the RISC-V psABI to improve the documentation. Unfortunately the handling of zero-width bitfields is a little weird, but the preference seems to be to just document what GCC does.


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

https://reviews.llvm.org/D60456