[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:9236
+    if (IsInt && Field1Ty && Field1Ty->isIntegerTy())
+      return false;
+    if (!Field1Ty) {
----------------
rjmccall wrote:
> 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.
I added a comment to document this. It's not something I'd expose in a public API, but I think it's defensible to catch this case outside of the helper. I had another look at refactoring but the readability just seemed to be reduced when pulling out all the checks to the caller (rather than catching the single case that detectFPCCEligibleStructHelper can't handle).


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9340
+        if (getContext().getTypeSize(QTy) > XLen && BitWidth <= XLen)
+          QTy = getContext().getIntTypeForBitwidth(XLen, false);
+        if (BitWidth == 0) {
----------------
rjmccall wrote:
> 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.
I'm afraid that's the rule as written, and what gcc seems to implement.


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

https://reviews.llvm.org/D60456