[clang][RISC-V] fixed fp calling convention for fpcc eligible structs for risc-v #110690
Conversation
| Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot added clang 
| @llvm/pr-subscribers-clang Author: Kamran Yousafzai (KamranYousafzai) ChangesThe code generated for calls with FPCC eligible structs as arguments doesn't consider the alignment with a bitfield, which results in a store crossing the boundary of the memory allocated using alloca, e.g. The generated IR is: Where, The patch trims the second member of the struct after taking into consideration the alignment and bitwidth to decide the appropriate integer type and the test shows the results of this patch. Note that the bug is seen only when Full diff: https://github.com/llvm/llvm-project/pull/110690.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/Targets/RISCV.cpp b/clang/lib/CodeGen/Targets/RISCV.cpp index fd72fe673b9b14..142371ffe27e54 100644 --- a/clang/lib/CodeGen/Targets/RISCV.cpp +++ b/clang/lib/CodeGen/Targets/RISCV.cpp @@ -224,6 +224,8 @@ bool RISCVABIInfo::detectFPCCEligibleStructHelper(QualType Ty, CharUnits CurOff, if (isEmptyRecord(getContext(), Ty, true, true)) return true; const RecordDecl *RD = RTy->getDecl(); + const Type *RT = RD->getTypeForDecl(); + unsigned Alignment = getContext().getTypeAlign(RT); // Unions aren't eligible unless they're empty (which is caught above). if (RD->isUnion()) return false; @@ -251,6 +253,15 @@ bool RISCVABIInfo::detectFPCCEligibleStructHelper(QualType Ty, CharUnits CurOff, // bitwidth is XLen or less. if (getContext().getTypeSize(QTy) > XLen && BitWidth <= XLen) QTy = getContext().getIntTypeForBitwidth(XLen, false); + // Trim type to alignment/bitwidth if that is possible + else if (getContext().getTypeSize(QTy) > Alignment && + getContext().getTypeSize(QTy) > BitWidth) { + bool isSigned = + FD->getType().getTypePtr()->hasSignedIntegerRepresentation(); + unsigned bits = + std::max(Alignment, (unsigned)llvm::PowerOf2Ceil(BitWidth)); + QTy = getContext().getIntTypeForBitwidth(bits, isSigned); + } if (BitWidth == 0) { ZeroWidthBitFieldCount++; continue; diff --git a/clang/test/CodeGen/RISCV/riscv-fpcc-struct.c b/clang/test/CodeGen/RISCV/riscv-fpcc-struct.c new file mode 100644 index 00000000000000..5d813aa05e60c6 --- /dev/null +++ b/clang/test/CodeGen/RISCV/riscv-fpcc-struct.c @@ -0,0 +1,21 @@ +// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5 +// RUN: %clang_cc1 -triple riscv64 -target-feature +f -emit-llvm -O3 %s -o - \ +// RUN: | FileCheck %s + + +struct __attribute__((packed, aligned(1))) S { + const float f0; + unsigned f1 : 1; +}; + +// CHECK-LABEL: define dso_local signext range(i32 0, 2) i32 @func( +// CHECK-SAME: float [[TMP0:%.*]], i8 [[TMP1:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] { +// CHECK-NEXT: [[ENTRY:.*:]] +// CHECK-NEXT: [[BF_CLEAR:%.*]] = and i8 [[TMP1]], 1 +// CHECK-NEXT: [[BF_CAST:%.*]] = zext nneg i8 [[BF_CLEAR]] to i32 +// CHECK-NEXT: ret i32 [[BF_CAST]] +// +unsigned func(struct S arg) +{ + return arg.f1; +} |
| @llvm/pr-subscribers-backend-risc-v Author: Kamran Yousafzai (KamranYousafzai) ChangesThe code generated for calls with FPCC eligible structs as arguments doesn't consider the alignment with a bitfield, which results in a store crossing the boundary of the memory allocated using alloca, e.g. The generated IR is: Where, The patch trims the second member of the struct after taking into consideration the alignment and bitwidth to decide the appropriate integer type and the test shows the results of this patch. Note that the bug is seen only when Full diff: https://github.com/llvm/llvm-project/pull/110690.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/Targets/RISCV.cpp b/clang/lib/CodeGen/Targets/RISCV.cpp index fd72fe673b9b14..142371ffe27e54 100644 --- a/clang/lib/CodeGen/Targets/RISCV.cpp +++ b/clang/lib/CodeGen/Targets/RISCV.cpp @@ -224,6 +224,8 @@ bool RISCVABIInfo::detectFPCCEligibleStructHelper(QualType Ty, CharUnits CurOff, if (isEmptyRecord(getContext(), Ty, true, true)) return true; const RecordDecl *RD = RTy->getDecl(); + const Type *RT = RD->getTypeForDecl(); + unsigned Alignment = getContext().getTypeAlign(RT); // Unions aren't eligible unless they're empty (which is caught above). if (RD->isUnion()) return false; @@ -251,6 +253,15 @@ bool RISCVABIInfo::detectFPCCEligibleStructHelper(QualType Ty, CharUnits CurOff, // bitwidth is XLen or less. if (getContext().getTypeSize(QTy) > XLen && BitWidth <= XLen) QTy = getContext().getIntTypeForBitwidth(XLen, false); + // Trim type to alignment/bitwidth if that is possible + else if (getContext().getTypeSize(QTy) > Alignment && + getContext().getTypeSize(QTy) > BitWidth) { + bool isSigned = + FD->getType().getTypePtr()->hasSignedIntegerRepresentation(); + unsigned bits = + std::max(Alignment, (unsigned)llvm::PowerOf2Ceil(BitWidth)); + QTy = getContext().getIntTypeForBitwidth(bits, isSigned); + } if (BitWidth == 0) { ZeroWidthBitFieldCount++; continue; diff --git a/clang/test/CodeGen/RISCV/riscv-fpcc-struct.c b/clang/test/CodeGen/RISCV/riscv-fpcc-struct.c new file mode 100644 index 00000000000000..5d813aa05e60c6 --- /dev/null +++ b/clang/test/CodeGen/RISCV/riscv-fpcc-struct.c @@ -0,0 +1,21 @@ +// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5 +// RUN: %clang_cc1 -triple riscv64 -target-feature +f -emit-llvm -O3 %s -o - \ +// RUN: | FileCheck %s + + +struct __attribute__((packed, aligned(1))) S { + const float f0; + unsigned f1 : 1; +}; + +// CHECK-LABEL: define dso_local signext range(i32 0, 2) i32 @func( +// CHECK-SAME: float [[TMP0:%.*]], i8 [[TMP1:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] { +// CHECK-NEXT: [[ENTRY:.*:]] +// CHECK-NEXT: [[BF_CLEAR:%.*]] = and i8 [[TMP1]], 1 +// CHECK-NEXT: [[BF_CAST:%.*]] = zext nneg i8 [[BF_CLEAR]] to i32 +// CHECK-NEXT: ret i32 [[BF_CAST]] +// +unsigned func(struct S arg) +{ + return arg.f1; +} |
| @llvm/pr-subscribers-clang-codegen Author: Kamran Yousafzai (KamranYousafzai) ChangesThe code generated for calls with FPCC eligible structs as arguments doesn't consider the alignment with a bitfield, which results in a store crossing the boundary of the memory allocated using alloca, e.g. The generated IR is: Where, The patch trims the second member of the struct after taking into consideration the alignment and bitwidth to decide the appropriate integer type and the test shows the results of this patch. Note that the bug is seen only when Full diff: https://github.com/llvm/llvm-project/pull/110690.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/Targets/RISCV.cpp b/clang/lib/CodeGen/Targets/RISCV.cpp index fd72fe673b9b14..142371ffe27e54 100644 --- a/clang/lib/CodeGen/Targets/RISCV.cpp +++ b/clang/lib/CodeGen/Targets/RISCV.cpp @@ -224,6 +224,8 @@ bool RISCVABIInfo::detectFPCCEligibleStructHelper(QualType Ty, CharUnits CurOff, if (isEmptyRecord(getContext(), Ty, true, true)) return true; const RecordDecl *RD = RTy->getDecl(); + const Type *RT = RD->getTypeForDecl(); + unsigned Alignment = getContext().getTypeAlign(RT); // Unions aren't eligible unless they're empty (which is caught above). if (RD->isUnion()) return false; @@ -251,6 +253,15 @@ bool RISCVABIInfo::detectFPCCEligibleStructHelper(QualType Ty, CharUnits CurOff, // bitwidth is XLen or less. if (getContext().getTypeSize(QTy) > XLen && BitWidth <= XLen) QTy = getContext().getIntTypeForBitwidth(XLen, false); + // Trim type to alignment/bitwidth if that is possible + else if (getContext().getTypeSize(QTy) > Alignment && + getContext().getTypeSize(QTy) > BitWidth) { + bool isSigned = + FD->getType().getTypePtr()->hasSignedIntegerRepresentation(); + unsigned bits = + std::max(Alignment, (unsigned)llvm::PowerOf2Ceil(BitWidth)); + QTy = getContext().getIntTypeForBitwidth(bits, isSigned); + } if (BitWidth == 0) { ZeroWidthBitFieldCount++; continue; diff --git a/clang/test/CodeGen/RISCV/riscv-fpcc-struct.c b/clang/test/CodeGen/RISCV/riscv-fpcc-struct.c new file mode 100644 index 00000000000000..5d813aa05e60c6 --- /dev/null +++ b/clang/test/CodeGen/RISCV/riscv-fpcc-struct.c @@ -0,0 +1,21 @@ +// NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --version 5 +// RUN: %clang_cc1 -triple riscv64 -target-feature +f -emit-llvm -O3 %s -o - \ +// RUN: | FileCheck %s + + +struct __attribute__((packed, aligned(1))) S { + const float f0; + unsigned f1 : 1; +}; + +// CHECK-LABEL: define dso_local signext range(i32 0, 2) i32 @func( +// CHECK-SAME: float [[TMP0:%.*]], i8 [[TMP1:%.*]]) local_unnamed_addr #[[ATTR0:[0-9]+]] { +// CHECK-NEXT: [[ENTRY:.*:]] +// CHECK-NEXT: [[BF_CLEAR:%.*]] = and i8 [[TMP1]], 1 +// CHECK-NEXT: [[BF_CAST:%.*]] = zext nneg i8 [[BF_CLEAR]] to i32 +// CHECK-NEXT: ret i32 [[BF_CAST]] +// +unsigned func(struct S arg) +{ + return arg.f1; +} |
KamranYousafzai force-pushed the mkamran/fpcc-eligible-structs branch from b751fd8 to 69d04c6 Compare KamranYousafzai changed the title 
clang/lib/CodeGen/Targets/RISCV.cpp Outdated
| if (getContext().getTypeSize(QTy) > XLen && BitWidth <= XLen) | ||
| QTy = getContext().getIntTypeForBitwidth(XLen, false); | ||
| // Trim type to alignment/bitwidth if that is possible | ||
| else if (getContext().getTypeSize(QTy) > Alignment && |
There was a problem hiding this comment.
Is the alignment relevant? Should we just round the bitfield size to the smallest power of 2 always? Even without the packed attribute, gcc -O0 emits sb while clang emits sw.
There was a problem hiding this comment.
You are right, the latest push reflects the change suggested. It now just checks against size of a byte to ensure the size at the very minimum is 8 bits.
KamranYousafzai force-pushed the mkamran/fpcc-eligible-structs branch 2 times, most recently from 8ca2d40 to f1da56e Compare KamranYousafzai force-pushed the mkamran/fpcc-eligible-structs branch from f1da56e to 2a41d57 Compare | // LP64: entry: | ||
| // | ||
| // LP64F-LP64D-LABEL: define dso_local void @f_float16_int64bf_s_arg | ||
| // LP64F-LP64D-SAME: (half [[TMP0:%.*]], i64 [[TMP1:%.*]]) #[[ATTR0]] { |
There was a problem hiding this comment.
I looked at the current IR for this.
We have
%struct.float16_int64bf_s = type <{ half, i32, [2 x i8] }> define dso_local void @f_float16_int64bf_s_arg(half %0, i64 %1) #0 !dbg !1528 { %3 = alloca %struct.float16_int64bf_s, align 8 %4 = getelementptr inbounds nuw <{ half, i64 }>, ptr %3, i32 0, i32 0 store half %0, ptr %4, align 8 %5 = getelementptr inbounds nuw <{ half, i64 }>, ptr %3, i32 0, i32 1 store i64 %1, ptr %5, align 2 #dbg_declare(ptr %3, !1535, !DIExpression(), !1536) ret void, !dbg !1537 } The struct type for the alloca is packed so it takes 8 bytes. Then we treat it as a packed struct of 10 bytes when we do the stores. So the i64 store went past the allocated memory.
| Hi @asb , could you please have a look and review the proposed changes? |
| @KamranYousafzai please can you follow the instructions about your email here: https://llvm.org/docs/DeveloperPolicy.html#github-email-address Then I will ensure this is merged. Sorry for this taking so long. |
Hi @lenary, I did change the email from private to public. Thanks! |
4 participants
The code generated for calls with FPCC eligible structs as arguments doesn't consider the bitfield, which results in a store crossing the boundary of the memory allocated using alloca, e.g.
For the code:
The generated IR is:
Where,
store i32 [[TMP1]], ptr [[TMP3]], align 1can be seen crossing the boundary of the allocated memory. If, the IR is seen after optimizations (EarlyCSEPass), the IR left is:The patch trims the second member of the struct after taking into consideration the bitwidth to decide the appropriate integer type and the test shows the results of this patch.
Note that the bug is seen only when
fextension is enabled for FPCC eligibility.