[CIR] Fix outdated bitfield iteration logic in accumulateFields #151741
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Andres-Salamanca
requested review from
andykaylor,
bcardosolopes,
lanza and
xlauko
as code owners
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clangir Author: None (Andres-Salamanca) ChangesThis PR fixes the outdated logic for accumulating bitfields in For reference, in ClangIR we already handle this correctly: Full diff: https://github.com/llvm/llvm-project/pull/151741.diff 2 Files Affected:
diff --git a/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp b/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp
index 0c8ff4bd807ad..ff9b38f79902c 100644
--- a/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenRecordLayoutBuilder.cpp
@@ -499,11 +499,7 @@ void CIRRecordLowering::accumulateFields() {
fieldEnd = recordDecl->field_end();
field != fieldEnd;) {
if (field->isBitField()) {
- RecordDecl::field_iterator start = field;
- // Iterate to gather the list of bitfields.
- for (++field; field != fieldEnd && field->isBitField(); ++field)
- ;
- field = accumulateBitFields(start, field);
+ field = accumulateBitFields(field, fieldEnd);
assert((field == fieldEnd || !field->isBitField()) &&
"Failed to accumulate all the bitfields");
} else if (!field->isZeroSize(astContext)) {
diff --git a/clang/test/CIR/CodeGen/bitfields.c b/clang/test/CIR/CodeGen/bitfields.c
index 869a7c98e2569..b2c7d1c0be926 100644
--- a/clang/test/CIR/CodeGen/bitfields.c
+++ b/clang/test/CIR/CodeGen/bitfields.c
@@ -71,12 +71,23 @@ typedef struct {
// LLVM-DAG: %struct.U = type <{ i8, i8, i8, i8, i64 }>
// OGCG-DAG: %struct.U = type <{ i8, i8, i8, i8, i64 }>
+typedef struct{
+ int a : 24;
+ char b;
+ int c: 30;
+} Clip;
+
+// CIR-DAG: !rec_Clip = !cir.record<struct "Clip" {!cir.array<!u8i x 3>, !s8i, !u32i}>
+// LLVM-DAG: %struct.Clip = type { [3 x i8], i8, i32 }
+// OGCG-DAG: %struct.Clip = type { [3 x i8], i8, i32 }
+
void def() {
A a;
D d;
S s;
T t;
U u;
+ Clip c;
}
int load_field(S* s) {
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the outdated logic for accumulating bitfields in
accumulateFields. The old approach remained after the algorithm was updated. A non-bitfield member would act as a barrier, causingaccumulateBitFieldsto receive an incomplete range of fields. As a result, it failed to accumulate them properly when clipping was necessary.For reference, in ClangIR we already handle this correctly:
https://github.com/llvm/clangir/blob/b647f4b97b1f936fd7700ec0fd0d896a12fe581b/clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp#L711-L714