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

John McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 8 11:57:16 PDT 2019


rjmccall added a comment.

I'm fine with proceeding with your best guess about what the ABI should be.



================
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
----------------
asb wrote:
> 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.
Defaulting to the integer ABI is fine.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9236
+    if (IsInt && Field1Ty && Field1Ty->isIntegerTy())
+      return false;
+    if (!Field1Ty) {
----------------
asb wrote:
> 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.
Okay.  It just seemed to me that responsibility was oddly split between the functions.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9340
+        if (getContext().getTypeSize(QTy) > XLen && BitWidth <= XLen)
+          QTy = getContext().getIntTypeForBitwidth(XLen, false);
+        if (BitWidth == 0) {
----------------
Okay.  So consecutive bit-fields are considered individually, not packed into a single storage unit and then considered?  Unfortunate ABI rule, but if it's what you have to implement, so be it.


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

https://reviews.llvm.org/D60456





More information about the cfe-commits mailing list