[PATCH] D69590: [RISCV] Fix ILP32D lowering for double+double/double+int return types

[James Clarke via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3d6c492d7a98: [RISCV] Fix ILP32D lowering for 
double+double/double+int return types (authored by jrtc27).

Changed prior to commit:
  https://reviews.llvm.org/D69590?vs=226981=237912#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69590

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/riscv32-ilp32d-abi.c


Index: clang/test/CodeGen/riscv32-ilp32d-abi.c
===
--- clang/test/CodeGen/riscv32-ilp32d-abi.c
+++ clang/test/CodeGen/riscv32-ilp32d-abi.c
@@ -280,3 +280,27 @@
 union double_u f_ret_double_u() {
   return (union double_u){1.0};
 }
+
+// Test that we don't incorrectly think double+int/double+double structs will
+// be returned indirectly and thus have an off-by-one error for the number of
+// GPRs available (this is an edge case when structs > 2*XLEN are still
+// returned in registers). This includes complex doubles, which are treated as
+// double+double structs by the ABI.
+
+// CHECK: define { double, i32 } 
@f_ret_double_int32_s_double_int32_s_just_sufficient_gprs(i32 %a, i32 %b, i32 
%c, i32 %d, i32 %e, i32 %f, i32 %g, double %0, i32 %1)
+struct double_int32_s f_ret_double_int32_s_double_int32_s_just_sufficient_gprs(
+int a, int b, int c, int d, int e, int f, int g, struct double_int32_s h) {
+  return (struct double_int32_s){1.0, 2};
+}
+
+// CHECK: define { double, double } 
@f_ret_double_double_s_double_int32_s_just_sufficient_gprs(i32 %a, i32 %b, i32 
%c, i32 %d, i32 %e, i32 %f, i32 %g, double %0, i32 %1)
+struct double_double_s 
f_ret_double_double_s_double_int32_s_just_sufficient_gprs(
+int a, int b, int c, int d, int e, int f, int g, struct double_int32_s h) {
+  return (struct double_double_s){1.0, 2.0};
+}
+
+// CHECK: define { double, double } 
@f_ret_doublecomplex_double_int32_s_just_sufficient_gprs(i32 %a, i32 %b, i32 
%c, i32 %d, i32 %e, i32 %f, i32 %g, double %0, i32 %1)
+double __complex__ f_ret_doublecomplex_double_int32_s_just_sufficient_gprs(
+int a, int b, int c, int d, int e, int f, int g, struct double_int32_s h) {
+  return 1.0;
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -9377,11 +9377,21 @@
 FI.getReturnInfo() = classifyReturnType(RetTy);
 
   // IsRetIndirect is true if classifyArgumentType indicated the value should
-  // be passed indirect or if the type size is greater than 2*xlen. e.g. fp128
-  // is passed direct in LLVM IR, relying on the backend lowering code to
-  // rewrite the argument list and pass indirectly on RV32.
-  bool IsRetIndirect = FI.getReturnInfo().getKind() == ABIArgInfo::Indirect ||
-   getContext().getTypeSize(RetTy) > (2 * XLen);
+  // be passed indirect, or if the type size is a scalar greater than 2*XLen
+  // and not a complex type with elements <= FLen. e.g. fp128 is passed direct
+  // in LLVM IR, relying on the backend lowering code to rewrite the argument
+  // list and pass indirectly on RV32.
+  bool IsRetIndirect = FI.getReturnInfo().getKind() == ABIArgInfo::Indirect;
+  if (!IsRetIndirect && RetTy->isScalarType() &&
+  getContext().getTypeSize(RetTy) > (2 * XLen)) {
+if (RetTy->isComplexType() && FLen) {
+  QualType EltTy = RetTy->getAs()->getElementType();
+  IsRetIndirect = getContext().getTypeSize(EltTy) > FLen;
+} else {
+  // This is a normal scalar > 2*XLen, such as fp128 on RV32.
+  IsRetIndirect = true;
+}
+  }
 
   // We must track the number of GPRs used in order to conform to the RISC-V
   // ABI, as integer scalars passed in registers should have signext/zeroext


Index: clang/test/CodeGen/riscv32-ilp32d-abi.c
===
--- clang/test/CodeGen/riscv32-ilp32d-abi.c
+++ clang/test/CodeGen/riscv32-ilp32d-abi.c
@@ -280,3 +280,27 @@
 union double_u f_ret_double_u() {
   return (union double_u){1.0};
 }
+
+// Test that we don't incorrectly think double+int/double+double structs will
+// be returned indirectly and thus have an off-by-one error for the number of
+// GPRs available (this is an edge case when structs > 2*XLEN are still
+// returned in registers). This includes complex doubles, which are treated as
+// double+double structs by the ABI.
+
+// CHECK: define { double, i32 } @f_ret_double_int32_s_double_int32_s_just_sufficient_gprs(i32 %a, i32 %b, i32 %c, i32 %d, i32 %e, i32 %f, i32 %g, double %0, i32 %1)
+struct double_int32_s f_ret_double_int32_s_double_int32_s_just_sufficient_gprs(
+int a, int b, int c, int d, int e, int f, int g, struct double_int32_s h) {
+  return (struct double_int32_s){1.0, 2};
+}
+
+// CHECK: define { double, double } 

[PATCH] D69590: [RISCV] Fix ILP32D lowering for double+double/double+int return types

[Sam Elliott via Phabricator via cfe-commits]

lenary accepted this revision.
lenary added a comment.

@jrtc27 It would be good to get this in for LLVM 10.0.

Having gone through the logic with @asb, we are both confident this is the 
right fix, and this patch does not require any additions at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69590



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

[PATCH] D69590: [RISCV] Fix ILP32D lowering for double+double/double+int return types

[Alex Bradbury via Phabricator via cfe-commits]

asb accepted this revision.
asb added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: luismarques.

This looks good to me, thanks James. I had a closer step through of the logic 
here to convince myself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69590



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

[PATCH] D69590: [RISCV] Fix ILP32D lowering for double+double/double+int return types

[Sam Elliott via Phabricator via cfe-commits]

lenary added a comment.

In D69590#1728628 , @asb wrote:

> Thanks James - won't this still leave problems for structs that need 
> flattening?


The tests in clang/test/CodeGen/riscv32-ilp32d-abi.c already include lots of 
cases that include flattening.

I suppose the one place they lack tests is probably a complex type passed 
within a struct, which would be good to add @jrtc27. Though, we should probably 
greatly expand the testing of `_Complex` within ABIs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69590



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

[PATCH] D69590: [RISCV] Fix ILP32D lowering for double+double/double+int return types

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.

Thanks James - won't this still leave problems for structs that need flattening?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69590



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

[PATCH] D69590: [RISCV] Fix ILP32D lowering for double+double/double+int return types

[James Clarke via Phabricator via cfe-commits]

jrtc27 created this revision.
jrtc27 added a reviewer: asb.
Herald added subscribers: cfe-commits, sameer.abuasal, pzheng, s.egerton, 
lenary, Jim, benna, psnobl, jocewei, PkmX, rkruppe, the_o, brucehoult, 
MartinMosbeck, rogfer01, edward-jones, zzheng, shiva0217, kito-cheng, niosHD, 
sabuasal, apazos, simoncook, johnrusso, rbar.
Herald added a project: clang.

Previously, since these aggregates are > 2*XLen, Clang would think they
were being returned indirectly and thus would decrease the number of
available GPRs available by 1. For long argument lists this could lead
to a struct argument incorrectly being passed indirectly.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69590

Files:
  clang/lib/CodeGen/TargetInfo.cpp
  clang/test/CodeGen/riscv32-ilp32d-abi.c


Index: clang/test/CodeGen/riscv32-ilp32d-abi.c
===
--- clang/test/CodeGen/riscv32-ilp32d-abi.c
+++ clang/test/CodeGen/riscv32-ilp32d-abi.c
@@ -280,3 +280,27 @@
 union double_u f_ret_double_u() {
   return (union double_u){1.0};
 }
+
+// Test that we don't incorrectly think double+int/double+double structs will
+// be returned indirectly and thus have an off-by-one error for the number of
+// GPRs available (this is an edge case when structs > 2*XLEN are still
+// returned in registers). This include complex doubles, which are treated as
+// double+double structs by the ABI.
+
+// CHECK: define { double, i32 } 
@f_ret_double_int32_s_double_int32_s_just_sufficient_gprs(i32 %a, i32 %b, i32 
%c, i32 %d, i32 %e, i32 %f, i32 %g, double %0, i32 %1)
+struct double_int32_s f_ret_double_int32_s_double_int32_s_just_sufficient_gprs(
+int a, int b, int c, int d, int e, int f, int g, struct double_int32_s h) {
+  return (struct double_int32_s){1.0, 2};
+}
+
+// CHECK: define { double, double } 
@f_ret_double_double_s_double_int32_s_just_sufficient_gprs(i32 %a, i32 %b, i32 
%c, i32 %d, i32 %e, i32 %f, i32 %g, double %0, i32 %1)
+struct double_double_s 
f_ret_double_double_s_double_int32_s_just_sufficient_gprs(
+int a, int b, int c, int d, int e, int f, int g, struct double_int32_s h) {
+  return (struct double_double_s){1.0, 2.0};
+}
+
+// CHECK: define { double, double } 
@f_ret_doublecomplex_double_int32_s_just_sufficient_gprs(i32 %a, i32 %b, i32 
%c, i32 %d, i32 %e, i32 %f, i32 %g, double %0, i32 %1)
+double __complex__ f_ret_doublecomplex_double_int32_s_just_sufficient_gprs(
+int a, int b, int c, int d, int e, int f, int g, struct double_int32_s h) {
+  return 1.0;
+}
Index: clang/lib/CodeGen/TargetInfo.cpp
===
--- clang/lib/CodeGen/TargetInfo.cpp
+++ clang/lib/CodeGen/TargetInfo.cpp
@@ -9291,11 +9291,21 @@
 FI.getReturnInfo() = classifyReturnType(RetTy);
 
   // IsRetIndirect is true if classifyArgumentType indicated the value should
-  // be passed indirect or if the type size is greater than 2*xlen. e.g. fp128
-  // is passed direct in LLVM IR, relying on the backend lowering code to
-  // rewrite the argument list and pass indirectly on RV32.
-  bool IsRetIndirect = FI.getReturnInfo().getKind() == ABIArgInfo::Indirect ||
-   getContext().getTypeSize(RetTy) > (2 * XLen);
+  // be passed indirect, or if the type size is a scalar greater than 2*XLen
+  // and not a complex type with elements <= FLen. e.g. fp128 is passed direct
+  // in LLVM IR, relying on the backend lowering code to rewrite the argument
+  // list and pass indirectly on RV32.
+  bool IsRetIndirect = FI.getReturnInfo().getKind() == ABIArgInfo::Indirect;
+  if (!IsRetIndirect && RetTy->isScalarType() &&
+  getContext().getTypeSize(RetTy) > (2 * XLen)) {
+if (RetTy->isComplexType() && FLen) {
+  QualType EltTy = RetTy->getAs()->getElementType();
+  IsRetIndirect = getContext().getTypeSize(EltTy) > FLen;
+} else {
+  // This is a normal scalar > 2*XLen, such as fp128 on RV32.
+  IsRetIndirect = true;
+}
+  }
 
   // We must track the number of GPRs used in order to conform to the RISC-V
   // ABI, as integer scalars passed in registers should have signext/zeroext


Index: clang/test/CodeGen/riscv32-ilp32d-abi.c
===
--- clang/test/CodeGen/riscv32-ilp32d-abi.c
+++ clang/test/CodeGen/riscv32-ilp32d-abi.c
@@ -280,3 +280,27 @@
 union double_u f_ret_double_u() {
   return (union double_u){1.0};
 }
+
+// Test that we don't incorrectly think double+int/double+double structs will
+// be returned indirectly and thus have an off-by-one error for the number of
+// GPRs available (this is an edge case when structs > 2*XLEN are still
+// returned in registers). This include complex doubles, which are treated as
+// double+double structs by the ABI.
+
+// CHECK: define { double, i32 } @f_ret_double_int32_s_double_int32_s_just_sufficient_gprs(i32 %a, i32 %b, i32 %c, i32 %d, i32 %e, i32 %f, i32 %g, double %0, i32 %1)
+struct