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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.
Herald added subscribers: vkmr, frasercrmck, evandro, luismarques, 
sameer.abuasal, s.egerton.

Your use of CoerceAndExpand seems fine; thanks for pinging me on it


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60456

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Bradbury via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL366450: [RISCV] Hard float ABI support (authored by asb, 
committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60456?vs=208477=210586#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60456

Files:
  cfe/trunk/lib/Basic/Targets/RISCV.cpp
  cfe/trunk/lib/Basic/Targets/RISCV.h
  cfe/trunk/lib/CodeGen/TargetInfo.cpp
  cfe/trunk/test/CodeGen/riscv32-ilp32-ilp32f-abi.c
  cfe/trunk/test/CodeGen/riscv32-ilp32-ilp32f-ilp32d-abi.c
  cfe/trunk/test/CodeGen/riscv32-ilp32d-abi.c
  cfe/trunk/test/CodeGen/riscv32-ilp32f-abi.c
  cfe/trunk/test/CodeGen/riscv32-ilp32f-ilp32d-abi.c
  cfe/trunk/test/CodeGen/riscv64-lp64-lp64f-abi.c
  cfe/trunk/test/CodeGen/riscv64-lp64-lp64f-lp64d-abi.c
  cfe/trunk/test/CodeGen/riscv64-lp64d-abi.c
  cfe/trunk/test/CodeGen/riscv64-lp64f-lp64d-abi.c
  cfe/trunk/test/Preprocessor/riscv-target-features.c

Index: cfe/trunk/lib/CodeGen/TargetInfo.cpp
===
--- cfe/trunk/lib/CodeGen/TargetInfo.cpp
+++ cfe/trunk/lib/CodeGen/TargetInfo.cpp
@@ -9188,25 +9188,44 @@
 namespace {
 class RISCVABIInfo : public DefaultABIInfo {
 private:
-  unsigned XLen; // Size of the integer ('x') registers in bits.
+  // Size of the integer ('x') registers in bits.
+  unsigned XLen;
+  // Size of the floating point ('f') registers in bits. Note that the target
+  // ISA might have a wider FLen than the selected ABI (e.g. an RV32IF target
+  // with soft float ABI has FLen==0).
+  unsigned FLen;
   static const int NumArgGPRs = 8;
+  static const int NumArgFPRs = 8;
+  bool detectFPCCEligibleStructHelper(QualType Ty, CharUnits CurOff,
+  llvm::Type *,
+  CharUnits ,
+  llvm::Type *,
+  CharUnits ) const;
 
 public:
-  RISCVABIInfo(CodeGen::CodeGenTypes , unsigned XLen)
-  : DefaultABIInfo(CGT), XLen(XLen) {}
+  RISCVABIInfo(CodeGen::CodeGenTypes , unsigned XLen, unsigned FLen)
+  : DefaultABIInfo(CGT), XLen(XLen), FLen(FLen) {}
 
   // DefaultABIInfo's classifyReturnType and classifyArgumentType are
   // non-virtual, but computeInfo is virtual, so we overload it.
   void computeInfo(CGFunctionInfo ) const override;
 
-  ABIArgInfo classifyArgumentType(QualType Ty, bool IsFixed,
-  int ) const;
+  ABIArgInfo classifyArgumentType(QualType Ty, bool IsFixed, int ,
+  int ) const;
   ABIArgInfo classifyReturnType(QualType RetTy) const;
 
   Address EmitVAArg(CodeGenFunction , Address VAListAddr,
 QualType Ty) const override;
 
   ABIArgInfo extendType(QualType Ty) const;
+
+  bool detectFPCCEligibleStruct(QualType Ty, llvm::Type *, CharUnits ,
+llvm::Type *, CharUnits ,
+int , int ) const;
+  ABIArgInfo coerceAndExpandFPCCEligibleStruct(llvm::Type *Field1Ty,
+   CharUnits Field1Off,
+   llvm::Type *Field2Ty,
+   CharUnits Field2Off) const;
 };
 } // end anonymous namespace
 
@@ -9228,18 +9247,214 @@
   // different for variadic arguments, we must also track whether we are
   // examining a vararg or not.
   int ArgGPRsLeft = IsRetIndirect ? NumArgGPRs - 1 : NumArgGPRs;
+  int ArgFPRsLeft = FLen ? NumArgFPRs : 0;
   int NumFixedArgs = FI.getNumRequiredArgs();
 
   int ArgNum = 0;
   for (auto  : FI.arguments()) {
 bool IsFixed = ArgNum < NumFixedArgs;
-ArgInfo.info = classifyArgumentType(ArgInfo.type, IsFixed, ArgGPRsLeft);
+ArgInfo.info =
+classifyArgumentType(ArgInfo.type, IsFixed, ArgGPRsLeft, ArgFPRsLeft);
 ArgNum++;
   }
 }
 
+// Returns true if the struct is a potential candidate for the floating point
+// calling convention. If this function returns true, the caller is
+// responsible for checking that if there is only a single field then that
+// field is a float.
+bool RISCVABIInfo::detectFPCCEligibleStructHelper(QualType Ty, CharUnits CurOff,
+  llvm::Type *,
+  CharUnits ,
+  llvm::Type *,
+  CharUnits ) const {
+  bool IsInt = Ty->isIntegralOrEnumerationType();
+  bool IsFloat = Ty->isRealFloatingType();
+
+  if (IsInt || IsFloat) {
+uint64_t Size = getContext().getTypeSize(Ty);
+if (IsInt && Size > XLen)
+  return false;
+// Can't be eligible if larger than the FP registers. Half precision isn't
+// currently supported on RISC-V and the ABI hasn't 

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

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Yes, I think you can commit.


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

https://reviews.llvm.org/D60456



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.

Thanks for the careful review John, I really appreciate it!


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

https://reviews.llvm.org/D60456



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Bradbury via Phabricator via cfe-commits]

asb updated this revision to Diff 208477.
asb marked 4 inline comments as done.
asb added a comment.

Tweaked a code comment.

Just to confirm, @rjmccall are you happy for me to commit this?


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

https://reviews.llvm.org/D60456

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/riscv32-ilp32-ilp32f-abi.c
  clang/test/CodeGen/riscv32-ilp32-ilp32f-ilp32d-abi.c
  clang/test/CodeGen/riscv32-ilp32d-abi.c
  clang/test/CodeGen/riscv32-ilp32f-abi.c
  clang/test/CodeGen/riscv32-ilp32f-ilp32d-abi.c
  clang/test/CodeGen/riscv64-lp64-lp64f-abi.c
  clang/test/CodeGen/riscv64-lp64-lp64f-lp64d-abi.c
  clang/test/CodeGen/riscv64-lp64d-abi.c
  clang/test/CodeGen/riscv64-lp64f-lp64d-abi.c
  clang/test/Preprocessor/riscv-target-features.c

Index: clang/test/Preprocessor/riscv-target-features.c
===
--- clang/test/Preprocessor/riscv-target-features.c
+++ clang/test/Preprocessor/riscv-target-features.c
@@ -47,3 +47,27 @@
 // RUN: %clang -target riscv64-unknown-linux-gnu -march=rv64ic -x c -E -dM %s \
 // RUN: -o - | FileCheck --check-prefix=CHECK-C-EXT %s
 // CHECK-C-EXT: __riscv_compressed 1
+
+// RUN: %clang -target riscv32-unknown-linux-gnu -march=rv32ifd -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-SOFT %s
+// RUN: %clang -target riscv64-unknown-linux-gnu -march=rv64ifd -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-SOFT %s
+// CHECK-SOFT: __riscv_float_abi_soft 1
+// CHECK-SOFT-NOT: __riscv_float_abi_single
+// CHECK-SOFT-NOT: __riscv_float_abi_double
+
+// RUN: %clang -target riscv32-unknown-linux-gnu -march=rv32ifd -mabi=ilp32f -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-SINGLE %s
+// RUN: %clang -target riscv64-unknown-linux-gnu -march=rv64ifd -mabi=lp64f -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-SINGLE %s
+// CHECK-SINGLE: __riscv_float_abi_single 1
+// CHECK-SINGLE-NOT: __riscv_float_abi_soft
+// CHECK-SINGLE-NOT: __riscv_float_abi_double
+
+// RUN: %clang -target riscv32-unknown-linux-gnu -march=rv32ifd -mabi=ilp32f -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-DOUBLE %s
+// RUN: %clang -target riscv64-unknown-linux-gnu -march=rv64ifd -mabi=lp64f -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-DOUBLE %s
+// CHECK-DOUBLE: __riscv_float_abi_double 1
+// CHECK-DOUBLE-NOT: __riscv_float_abi_soft
+// CHECK-DOUBLE-NOT: __riscv_float_abi_single
Index: clang/test/CodeGen/riscv64-lp64f-lp64d-abi.c
===
--- /dev/null
+++ clang/test/CodeGen/riscv64-lp64f-lp64d-abi.c
@@ -0,0 +1,265 @@
+// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-abi lp64f -emit-llvm %s -o - \
+// RUN: | FileCheck %s
+// RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-abi lp64d -emit-llvm %s -o - \
+// RUN: | FileCheck %s
+
+#include 
+
+// Verify that the tracking of used GPRs and FPRs works correctly by checking
+// that small integers are sign/zero extended when passed in registers.
+
+// Floats are passed in FPRs, so argument 'i' will be passed zero-extended
+// because it will be passed in a GPR.
+
+// CHECK: define void @f_fpr_tracking(float %a, float %b, float %c, float %d, float %e, float %f, float %g, float %h, i8 zeroext %i)
+void f_fpr_tracking(float a, float b, float c, float d, float e, float f,
+float g, float h, uint8_t i) {}
+
+// Check that fp, fp+fp, and int+fp structs are lowered correctly. These will
+// be passed in FPR, FPR+FPR, or GPR+FPR regs if sufficient registers are
+// available the widths are <= XLEN and FLEN, and should be expanded to
+// separate arguments in IR. They are passed by the same rules for returns,
+// but will be lowered to simple two-element structs if necessary (as LLVM IR
+// functions cannot return multiple values).
+
+// A struct containing just one floating-point real is passed as though it
+// were a standalone floating-point real.
+
+struct float_s { float f; };
+
+// CHECK: define void @f_float_s_arg(float)
+void f_float_s_arg(struct float_s a) {}
+
+// CHECK: define float @f_ret_float_s()
+struct float_s f_ret_float_s() {
+  return (struct float_s){1.0};
+}
+
+// A struct containing a float and any number of zero-width bitfields is
+// passed as though it were a standalone floating-point real.
+
+struct zbf_float_s { int : 0; float f; };
+struct zbf_float_zbf_s { int : 0; float f; int : 0; };
+
+// CHECK: define void @f_zbf_float_s_arg(float)
+void f_zbf_float_s_arg(struct zbf_float_s a) {}
+
+// CHECK: define float @f_ret_zbf_float_s()
+struct zbf_float_s f_ret_zbf_float_s() {
+  return (struct zbf_float_s){1.0};
+}
+
+// CHECK: define void @f_zbf_float_zbf_s_arg(float)
+void f_zbf_float_zbf_s_arg(struct zbf_float_zbf_s a) {}
+
+// CHECK: define float 

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

[John McCall via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via Phabricator via cfe-commits]

rogfer01 added a comment.

> As noted in another comment, it's not entirely clear what zero-width bitfield 
> behaviour to match (see here 
> )
>  as GCC seems buggy and the ABI is under-specified. Ideally I'd like to land 
> this patch and follow-up to adjust the zero-width bitfield behaviour if 
> necessary once that psABI issue is resolved.

Agreed, I presume the original intent in the psABI was to have C and C++ behave 
the same. We're siding with gcc in this patch but it should not be difficult to 
change if the psABI resolves this in favour of g++.


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

https://reviews.llvm.org/D60456



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Bradbury via Phabricator via cfe-commits]

asb updated this revision to Diff 208392.
asb marked an inline comment as done.
asb added a comment.

Updated to address comment typo picked up by @rogfer01 (thanks!).

As noted in another comment, it's not entirely clear what zero-width bitfield 
behaviour to match (see here 
)
 as GCC seems buggy and the ABI is under-specified. Ideally I'd like to land 
this patch and follow-up to adjust the zero-width bitfield behaviour if 
necessary once that psABI issue is resolved.


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

https://reviews.llvm.org/D60456

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/riscv32-ilp32-ilp32f-abi.c
  clang/test/CodeGen/riscv32-ilp32-ilp32f-ilp32d-abi.c
  clang/test/CodeGen/riscv32-ilp32d-abi.c
  clang/test/CodeGen/riscv32-ilp32f-abi.c
  clang/test/CodeGen/riscv32-ilp32f-ilp32d-abi.c
  clang/test/CodeGen/riscv64-lp64-lp64f-abi.c
  clang/test/CodeGen/riscv64-lp64-lp64f-lp64d-abi.c
  clang/test/CodeGen/riscv64-lp64d-abi.c
  clang/test/CodeGen/riscv64-lp64f-lp64d-abi.c
  clang/test/Preprocessor/riscv-target-features.c

Index: clang/test/Preprocessor/riscv-target-features.c
===
--- clang/test/Preprocessor/riscv-target-features.c
+++ clang/test/Preprocessor/riscv-target-features.c
@@ -47,3 +47,27 @@
 // RUN: %clang -target riscv64-unknown-linux-gnu -march=rv64ic -x c -E -dM %s \
 // RUN: -o - | FileCheck --check-prefix=CHECK-C-EXT %s
 // CHECK-C-EXT: __riscv_compressed 1
+
+// RUN: %clang -target riscv32-unknown-linux-gnu -march=rv32ifd -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-SOFT %s
+// RUN: %clang -target riscv64-unknown-linux-gnu -march=rv64ifd -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-SOFT %s
+// CHECK-SOFT: __riscv_float_abi_soft 1
+// CHECK-SOFT-NOT: __riscv_float_abi_single
+// CHECK-SOFT-NOT: __riscv_float_abi_double
+
+// RUN: %clang -target riscv32-unknown-linux-gnu -march=rv32ifd -mabi=ilp32f -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-SINGLE %s
+// RUN: %clang -target riscv64-unknown-linux-gnu -march=rv64ifd -mabi=lp64f -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-SINGLE %s
+// CHECK-SINGLE: __riscv_float_abi_single 1
+// CHECK-SINGLE-NOT: __riscv_float_abi_soft
+// CHECK-SINGLE-NOT: __riscv_float_abi_double
+
+// RUN: %clang -target riscv32-unknown-linux-gnu -march=rv32ifd -mabi=ilp32f -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-DOUBLE %s
+// RUN: %clang -target riscv64-unknown-linux-gnu -march=rv64ifd -mabi=lp64f -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-DOUBLE %s
+// CHECK-DOUBLE: __riscv_float_abi_double 1
+// CHECK-DOUBLE-NOT: __riscv_float_abi_soft
+// CHECK-DOUBLE-NOT: __riscv_float_abi_single
Index: clang/test/CodeGen/riscv64-lp64f-lp64d-abi.c
===
--- /dev/null
+++ clang/test/CodeGen/riscv64-lp64f-lp64d-abi.c
@@ -0,0 +1,265 @@
+// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-abi lp64f -emit-llvm %s -o - \
+// RUN: | FileCheck %s
+// RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-abi lp64d -emit-llvm %s -o - \
+// RUN: | FileCheck %s
+
+#include 
+
+// Verify that the tracking of used GPRs and FPRs works correctly by checking
+// that small integers are sign/zero extended when passed in registers.
+
+// Floats are passed in FPRs, so argument 'i' will be passed zero-extended
+// because it will be passed in a GPR.
+
+// CHECK: define void @f_fpr_tracking(float %a, float %b, float %c, float %d, float %e, float %f, float %g, float %h, i8 zeroext %i)
+void f_fpr_tracking(float a, float b, float c, float d, float e, float f,
+float g, float h, uint8_t i) {}
+
+// Check that fp, fp+fp, and int+fp structs are lowered correctly. These will
+// be passed in FPR, FPR+FPR, or GPR+FPR regs if sufficient registers are
+// available the widths are <= XLEN and FLEN, and should be expanded to
+// separate arguments in IR. They are passed by the same rules for returns,
+// but will be lowered to simple two-element structs if necessary (as LLVM IR
+// functions cannot return multiple values).
+
+// A struct containing just one floating-point real is passed as though it
+// were a standalone floating-point real.
+
+struct float_s { float f; };
+
+// CHECK: define void @f_float_s_arg(float)
+void f_float_s_arg(struct float_s a) {}
+
+// CHECK: define float @f_ret_float_s()
+struct float_s f_ret_float_s() {
+  return (struct float_s){1.0};
+}
+
+// A struct containing a float and any number of zero-width bitfields is
+// passed as though it were a standalone floating-point real.
+
+struct zbf_float_s { int : 0; float f; };
+struct zbf_float_zbf_s { int : 0; float f; int : 0; };
+
+// CHECK: define void 

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

[Alex Bradbury via Phabricator via cfe-commits]

asb marked an inline comment as done.
asb added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:9352
+return false;
+  // As a quirk of the ABI, zero-width bitfields aren't ignored for fp+fp
+  // or int+fp structs, but are ignored for a struct with an fp field and

rogfer01 wrote:
> I found some mismatch in behaviour between gcc and g++ that we may want to 
> address in the psABI first.
> 
> For instance, given the following struct (I'm using gcc 8.3.0)
> 
> ```lang=cpp
> // t.c
> struct A
> {
>   int :0;
>   double d;
>   int :0;
>   long x;
>   int :0;
> };
> 
> extern void bar(struct A);
> void foo(struct A a)
> {
>   a.d =- a.d;
>   a.x += 1;
>   return bar(a);
> }
> ```
> 
> we are emitting this
> 
> ```
> $ clang --target=riscv64 -march=rv64gc -mabi=lp64d -S -o-  t.c -O2  
> ...
> foo:# @foo
> # %bb.0:# %entry
> addia2, zero, -1
> sllia2, a2, 63
> xor a0, a0, a2
> addia1, a1, 1
> tailbar
> ```
> 
> which matches with what g++ does (i.e in both cases `a0` is `a.d` and `a1` is 
> `a.x`)
> 
> ```
> $ ./riscv64-unknown-linux-gnu-g++ -S -O2 -o- -x c test.cc
> ...
> foo:
>   fmv.d.x fa5,a0
>   addisp,sp,-16
>   fneg.d  fa5,fa5
>   addia1,a1,1
>   addisp,sp,16
>   fmv.x.d a0,fa5
>   tailbar
> ```
> 
> But I found a mismatch while using C++. Clang emits the same for C and C++ 
> (modulo `.cfi` stuff)
> 
> ```
> $ clang --target=riscv64 -march=rv64gc -mabi=lp64d -S -o-  -x c++ t.c -O2  
> _Z3foo1A:   # @_Z3foo1A
> .cfi_startproc
> # %bb.0:# %entry
> addia2, zero, -1
> sllia2, a2, 63
> xor a0, a0, a2
> addia1, a1, 1
> .cfi_def_cfa_offset 0
> tail_Z3bar1A
> ```
> 
> But g++ seems to ignore the zero-width bitfields: `fa0` is  `a.d` and `a0` is 
> `a.x`
> 
> ```
> $ riscv64-unknown-linux-gnu-g++  -S -O2 -x c++ t.c -o-
> ...
> _Z3foo1A:
> .LFB0:
> .cfi_startproc
> fneg.d  fa0,fa0
> addisp,sp,-16
> .cfi_def_cfa_offset 16
> addia0,a0,1
> addisp,sp,16
> .cfi_def_cfa_offset 0
> tail_Z3bar1A
> .cfi_endproc
> ```
> 
> This is a bit worrying as it might complicate interoperability between C and 
> C++ (I tried wrapping everything inside an `extern "C"` just in case but it 
> didn't change g++'s behaviour).
> 
> Do you mind to confirm this issue?
Thanks, I'm seeing this in GCC 9.1.0 as well. I left[ a 
comment](https://github.com/riscv/riscv-elf-psabi-doc/issues/99#issuecomment-509233798)
 on the relevant psABI issue. It seems there is a GCC bug here, but hopefully 
someone can confirm what the "correct" behaviour is.


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

https://reviews.llvm.org/D60456



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alex Bradbury via Phabricator via cfe-commits]

asb updated this revision to Diff 208308.
asb marked 7 inline comments as done.
asb retitled this revision from "[RISCV][WIP/RFC] Hard float ABI support" to 
"[RISCV] Hard float ABI support".
asb edited the summary of this revision.
asb added a comment.
Herald added subscribers: lenary, Jim, MaskRay.

Address all review comments from @rjmccall. Bitfield handling matches observed 
behaviour of GCC and I have an active PR 
 to properly document 
this in the RISC-V psabi. Tests are updated to check this behaviour.

Many thanks for the review comments - do you think this is ready to land?


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

https://reviews.llvm.org/D60456

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/riscv32-ilp32-ilp32f-abi.c
  clang/test/CodeGen/riscv32-ilp32-ilp32f-ilp32d-abi.c
  clang/test/CodeGen/riscv32-ilp32d-abi.c
  clang/test/CodeGen/riscv32-ilp32f-abi.c
  clang/test/CodeGen/riscv32-ilp32f-ilp32d-abi.c
  clang/test/CodeGen/riscv64-lp64-lp64f-abi.c
  clang/test/CodeGen/riscv64-lp64-lp64f-lp64d-abi.c
  clang/test/CodeGen/riscv64-lp64d-abi.c
  clang/test/CodeGen/riscv64-lp64f-lp64d-abi.c
  clang/test/Preprocessor/riscv-target-features.c

Index: clang/test/Preprocessor/riscv-target-features.c
===
--- clang/test/Preprocessor/riscv-target-features.c
+++ clang/test/Preprocessor/riscv-target-features.c
@@ -47,3 +47,27 @@
 // RUN: %clang -target riscv64-unknown-linux-gnu -march=rv64ic -x c -E -dM %s \
 // RUN: -o - | FileCheck --check-prefix=CHECK-C-EXT %s
 // CHECK-C-EXT: __riscv_compressed 1
+
+// RUN: %clang -target riscv32-unknown-linux-gnu -march=rv32ifd -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-SOFT %s
+// RUN: %clang -target riscv64-unknown-linux-gnu -march=rv64ifd -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-SOFT %s
+// CHECK-SOFT: __riscv_float_abi_soft 1
+// CHECK-SOFT-NOT: __riscv_float_abi_single
+// CHECK-SOFT-NOT: __riscv_float_abi_double
+
+// RUN: %clang -target riscv32-unknown-linux-gnu -march=rv32ifd -mabi=ilp32f -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-SINGLE %s
+// RUN: %clang -target riscv64-unknown-linux-gnu -march=rv64ifd -mabi=lp64f -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-SINGLE %s
+// CHECK-SINGLE: __riscv_float_abi_single 1
+// CHECK-SINGLE-NOT: __riscv_float_abi_soft
+// CHECK-SINGLE-NOT: __riscv_float_abi_double
+
+// RUN: %clang -target riscv32-unknown-linux-gnu -march=rv32ifd -mabi=ilp32f -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-DOUBLE %s
+// RUN: %clang -target riscv64-unknown-linux-gnu -march=rv64ifd -mabi=lp64f -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-DOUBLE %s
+// CHECK-DOUBLE: __riscv_float_abi_double 1
+// CHECK-DOUBLE-NOT: __riscv_float_abi_soft
+// CHECK-DOUBLE-NOT: __riscv_float_abi_single
Index: clang/test/CodeGen/riscv64-lp64f-lp64d-abi.c
===
--- /dev/null
+++ clang/test/CodeGen/riscv64-lp64f-lp64d-abi.c
@@ -0,0 +1,265 @@
+// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-abi lp64f -emit-llvm %s -o - \
+// RUN: | FileCheck %s
+// RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-abi lp64d -emit-llvm %s -o - \
+// RUN: | FileCheck %s
+
+#include 
+
+// Verify that the tracking of used GPRs and FPRs works correctly by checking
+// that small integers are sign/zero extended when passed in registers.
+
+// Floats are passed in FPRs, so argument 'i' will be passed zero-extended
+// because it will be passed in a GPR.
+
+// CHECK: define void @f_fpr_tracking(float %a, float %b, float %c, float %d, float %e, float %f, float %g, float %h, i8 zeroext %i)
+void f_fpr_tracking(float a, float b, float c, float d, float e, float f,
+float g, float h, uint8_t i) {}
+
+// Check that fp, fp+fp, and int+fp structs are lowered correctly. These will
+// be passed in FPR, FPR+FPR, or GPR+FPR regs if sufficient registers are
+// available the widths are <= XLEN and FLEN, and should be expanded to
+// separate arguments in IR. They are passed by the same rules for returns,
+// but will be lowered to simple two-element structs if necessary (as LLVM IR
+// functions cannot return multiple values).
+
+// A struct containing just one floating-point real is passed as though it
+// were a standalone floating-point real.
+
+struct float_s { float f; };
+
+// CHECK: define void @f_float_s_arg(float)
+void f_float_s_arg(struct float_s a) {}
+
+// CHECK: define float @f_ret_float_s()
+struct float_s f_ret_float_s() {
+  return (struct float_s){1.0};
+}
+
+// A struct containing a float and any number of zero-width bitfields is
+// passed as though it were a standalone floating-point real.
+
+struct zbf_float_s { int : 0; 

[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  = 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via Phabricator via cfe-commits]

rogfer01 added inline comments.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:9352
+return false;
+  // As a quirk of the ABI, zero-width bitfields aren't ignored for fp+fp
+  // or int+fp structs, but are ignored for a struct with an fp field and

I found some mismatch in behaviour between gcc and g++ that we may want to 
address in the psABI first.

For instance, given the following struct (I'm using gcc 8.3.0)

```lang=cpp
// t.c
struct A
{
  int :0;
  double d;
  int :0;
  long x;
  int :0;
};

extern void bar(struct A);
void foo(struct A a)
{
  a.d =- a.d;
  a.x += 1;
  return bar(a);
}
```

we are emitting this

```
$ clang --target=riscv64 -march=rv64gc -mabi=lp64d -S -o-  t.c -O2  
...
foo:# @foo
# %bb.0:# %entry
addia2, zero, -1
sllia2, a2, 63
xor a0, a0, a2
addia1, a1, 1
tailbar
```

which matches with what g++ does (i.e in both cases `a0` is `a.d` and `a1` is 
`a.x`)

```
$ ./riscv64-unknown-linux-gnu-g++ -S -O2 -o- -x c test.cc
...
foo:
fmv.d.x fa5,a0
addisp,sp,-16
fneg.d  fa5,fa5
addia1,a1,1
addisp,sp,16
fmv.x.d a0,fa5
tailbar
```

But I found a mismatch while using C++. Clang emits the same for C and C++ 
(modulo `.cfi` stuff)

```
$ clang --target=riscv64 -march=rv64gc -mabi=lp64d -S -o-  -x c++ t.c -O2  
_Z3foo1A:   # @_Z3foo1A
.cfi_startproc
# %bb.0:# %entry
addia2, zero, -1
sllia2, a2, 63
xor a0, a0, a2
addia1, a1, 1
.cfi_def_cfa_offset 0
tail_Z3bar1A
```

But g++ seems to ignore the zero-width bitfields: `fa0` is  `a.d` and `a0` is 
`a.x`

```
$ riscv64-unknown-linux-gnu-g++  -S -O2 -x c++ t.c -o-
...
_Z3foo1A:
.LFB0:
.cfi_startproc
fneg.d  fa0,fa0
addisp,sp,-16
.cfi_def_cfa_offset 16
addia0,a0,1
addisp,sp,16
.cfi_def_cfa_offset 0
tail_Z3bar1A
.cfi_endproc
```

This is a bit worrying as it might complicate interoperability between C and 
C++ (I tried wrapping everything inside an `extern "C"` just in case but it 
didn't change g++'s behaviour).

Do you mind to confirm this issue?



Comment at: clang/lib/CodeGen/TargetInfo.cpp:9394
+// Call getCoerceAndExpand for the two-element flattened struct described by
+// Field1Ty, Filed2Ty, Filed2Off. This method will create an appropriate
+// coerceToType and unpaddedCoerceToType.

Typo  in `Filed2Ty` and `Filed2Off`


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

https://reviews.llvm.org/D60456



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits