[PATCH] D60456: [RISCV][WIP/RFC] Hard float ABI support

[John McCall via Phabricator via cfe-commits]

rjmccall 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

Is this the only consideration for floating-point types?  Clang does have 
increasing support for `half` / various `float16` types.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:9236
+if (IsInt && Field1Ty && Field1Ty->isIntegerTy())
+  return false;
+if (!Field1Ty) {

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?



Comment at: clang/lib/CodeGen/TargetInfo.cpp:9258
+assert(CurOff.isZero() && "Unexpected offset for first field");
+Field2Ty = CGT.ConvertType(EltTy);
+Field2Off = getContext().getTypeSizeInChars(EltTy);

`Field2Ty = Field1Ty`, please.



Comment at: clang/lib/CodeGen/TargetInfo.cpp:9288
+  return false;
+for (const FieldDecl *FD : RD->fields()) {
+  const ASTRecordLayout  = getContext().getASTRecordLayout(RD);

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?


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][WIP/RFC] Hard float ABI support

[Alex Bradbury via Phabricator via cfe-commits]

asb added inline comments.



Comment at: lib/CodeGen/TargetInfo.cpp:9223
+
+  bool IsInt = Ty->isIntegralOrEnumerationType();
+  bool IsFloat = Ty->isRealFloatingType();

rjmccall wrote:
> Should this include pointers?  Pointers are often interchangeably with 
> integers by ABIs.
> 
> The same question also applies to C++ references and data-member-pointer 
> types, and maybe others that I'm not thinking of.
The ABI doesn't consider pointers and integers interchangeable in this case. An 
"integer" is indeed an integer.


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][WIP/RFC] Hard float ABI support

[Alex Bradbury via Phabricator via cfe-commits]

asb updated this revision to Diff 198797.
asb marked 3 inline comments as done.
asb added a comment.

Update:

- Expanded and improved tests
- Set ABI defines
- Remove errant TODO
- Use alignTo

Still to do:

- Review and test bitfield handling (which is likely incomplete)


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

Index: clang/test/CodeGen/riscv64-lp64f-lp64d-abi.c
===
--- /dev/null
+++ clang/test/CodeGen/riscv64-lp64f-lp64d-abi.c
@@ -0,0 +1,222 @@
+// 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};
+}
+
+// Check that structs containing two float values (FLEN <= width) are expanded
+// provided sufficient FPRs are available.
+
+struct float_float_s { float f; float g; };
+
+// CHECK: define void @f_float_float_s_arg(float, float)
+void f_float_float_s_arg(struct float_float_s a) {}
+
+// CHECK: define { float, float } @f_ret_float_float_s()
+struct float_float_s f_ret_float_float_s() {
+  return (struct float_float_s){1.0, 2.0};
+}
+
+// CHECK: define void @f_float_float_s_arg_insufficient_fprs(float %a, float %b, float %c, float %d, float %e, float %f, float %g, i64 %h.coerce)
+void f_float_float_s_arg_insufficient_fprs(float a, float b, float c, float d,
+float e, float f, float g, struct float_float_s h) {}
+
+// Check that structs containing int+float values are expanded, provided
+// sufficient FPRs and GPRs are available. The integer components are neither
+// sign or zero-extended.
+
+struct float_int8_s { float f; int8_t i; };
+struct float_uint8_s { float f; uint8_t i; };
+struct float_int32_s { float f; int32_t i; };
+struct float_int64_s { float f; int64_t i; };
+
+// CHECK: define void @f_float_int8_s_arg(float, i8)
+void f_float_int8_s_arg(struct float_int8_s a) {}
+
+// CHECK: define { float, i8 } @f_ret_float_int8_s()
+struct float_int8_s f_ret_float_int8_s() {
+  return (struct float_int8_s){1.0, 2};
+}
+
+// CHECK: define void @f_float_uint8_s_arg(float, i8)
+void f_float_uint8_s_arg(struct float_uint8_s a) {}
+
+// CHECK: define { float, i8 } @f_ret_float_uint8_s()
+struct float_uint8_s f_ret_float_uint8_s() {
+  return (struct float_uint8_s){1.0, 2};
+}
+
+// CHECK: define void @f_float_int32_s_arg(float, i32)
+void f_float_int32_s_arg(struct float_int32_s a) {}
+
+// CHECK: define { float, i32 } @f_ret_float_int32_s()
+struct float_int32_s f_ret_float_int32_s() {
+  return (struct float_int32_s){1.0, 2};
+}
+
+// CHECK: define void @f_float_int64_s_arg(float, i64)
+void f_float_int64_s_arg(struct float_int64_s a) {}
+
+// CHECK: define { float, i64 } @f_ret_float_int64_s()
+struct float_int64_s f_ret_float_int64_s() {
+  return (struct float_int64_s){1.0, 2};
+}
+
+// CHECK: define void @f_float_int8_s_arg_insufficient_gprs(i32 signext %a, i32 signext %b, i32 signext %c, i32 signext %d, i32 signext %e, i32 signext %f, i32 signext %g, i32 

[PATCH] D60456: [RISCV][WIP/RFC] Hard float ABI support

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Basic/Targets/RISCV.h:102
 // TODO: support lp64f and lp64d ABIs.
-if (Name == "lp64") {
+if (Name == "lp64" || Name == "lp64f" || Name == "lp64d") {
   ABI = Name;

You can remove the TODO here, assuming that this CC support is all that's 
necessary to support these ABIs.



Comment at: lib/CodeGen/TargetInfo.cpp:9223
+
+  bool IsInt = Ty->isIntegralOrEnumerationType();
+  bool IsFloat = Ty->isRealFloatingType();

Should this include pointers?  Pointers are often interchangeably with integers 
by ABIs.

The same question also applies to C++ references and data-member-pointer types, 
and maybe others that I'm not thinking of.



Comment at: lib/CodeGen/TargetInfo.cpp:9298
+return true;
+  }
+

This definitely needs to handle bit-fields in some way.



Comment at: lib/CodeGen/TargetInfo.cpp:9352
+  CharUnits::fromQuantity(getDataLayout().getTypeStoreSize(Field1Ty));
+  CharUnits Field2OffNoPadNoPack = std::max(Field2Align, Field1Size);
+

This should use `alignTo`, not `max`.  It's possible that they're equivalent 
for the narrow cases that can come out of all the checks above, but it's both 
clearer and safer to use the correct computation.



Comment at: lib/CodeGen/TargetInfo.cpp:9374
+
+  return ABIArgInfo::getCoerceAndExpand(CoerceToType, UnpaddedCoerceToType);
+}

This seems like a reasonable use of coerce-and-expand.


Repository:
  rC Clang

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][WIP/RFC] Hard float ABI support

[Alex Bradbury via Phabricator via cfe-commits]

asb created this revision.
asb added a reviewer: rjmccall.
Herald added subscribers: benna, psnobl, jocewei, PkmX, rkruppe, the_o, 
brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, shiva0217, 
kito-cheng, niosHD, sabuasal, apazos, simoncook, johnrusso, rbar.
Herald added a project: clang.

The RISC-V hard float calling convention 

 requires the frontend to:

- Detect cases where, once "flattened", a struct can be passed using int+fp or 
fp+fp registers under the hard float ABI and coerce to the appropriate type(s)
- Track usage of GPRs and FPRs in order to gate the above, and to determine 
when signext/zeroext attributes must be added to integer scalars

This patch attempts to do this in compliance with the documented ABI, and uses 
ABIArgInfo::CoerceAndExpand in order to do this. @rjmccall, as author of that 
code I've tagged you as reviewer for initial feedback on my usage.

Note that a previous version of the ABI indicated that when passing an int+fp 
struct using a GPR+FPR, the int would need to be sign or zero-extended 
appropriately. GCC never did this 
 and the ABI was changed 
, which makes life easier 
as ABIArgInfo::CoerceAndExpand can't currently handle sign/zero-extension 
attributes.

I'm sharing just in case there are any concerns about this general approach. 
Known deficiencies in this patch are all related to testing:

- Needs more basic ilp32d/lp64f/lp64d tests
- Not enough coverage of bitfields in structs, packed+aligned structs etc


Repository:
  rC Clang

https://reviews.llvm.org/D60456

Files:
  lib/Basic/Targets/RISCV.h
  lib/CodeGen/TargetInfo.cpp
  test/CodeGen/riscv32-ilp32-ilp32f-abi.c
  test/CodeGen/riscv32-ilp32-ilp32f-ilp32d-abi.c
  test/CodeGen/riscv32-ilp32f-abi.c
  test/CodeGen/riscv32-ilp32f-ilp32d-abi.c

Index: test/CodeGen/riscv32-ilp32f-ilp32d-abi.c
===
--- /dev/null
+++ test/CodeGen/riscv32-ilp32f-ilp32d-abi.c
@@ -0,0 +1,230 @@
+// RUN: %clang_cc1 -triple riscv32 -target-feature +f -target-abi ilp32f -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};
+}
+
+// Check that structs containing two float values (FLEN <= width) are expanded
+// provided sufficient FPRs are available.
+
+struct float_float_s { float f; float g; };
+
+// CHECK: define void @f_float_float_s_arg(float, float)
+void f_float_float_s_arg(struct float_float_s a) {}
+
+// CHECK: define { float, float } @f_ret_float_float_s()
+struct float_float_s f_ret_float_float_s() {
+  return (struct float_float_s){1.0, 2.0};
+}
+
+// CHECK: define void @f_float_float_s_arg_insufficient_fprs(float %a, float %b, float %c, float %d, float %e, float %f, float %g, [2 x i32] %h.coerce)
+void f_float_float_s_arg_insufficient_fprs(float a, float b, float c, float d, 
+float e, float f, float g, struct float_float_s h) {}
+
+// Check that structs containing int+float values are expanded, provided
+// sufficient FPRs and GPRs are available. The integer components are neither
+// sign or zero-extended.
+
+struct float_int8_s { float f; int8_t i; };
+struct float_uint8_s { float f; uint8_t i; };
+struct float_int32_s { float f; int32_t i; };
+struct float_int64_s { float f; int64_t i; };
+
+// CHECK: define void @f_float_int8_s_arg(float, i8)
+void f_float_int8_s_arg(struct float_int8_s a) {}
+
+// CHECK: define { float, i8 } @f_ret_float_int8_s()
+struct float_int8_s f_ret_float_int8_s() {
+  return (struct