[PATCH] D79052: [clang codegen] Fix alignment of "Address" for incomplete array pointer.
This revision was automatically updated to reflect the committed changes. Closed by commit rGbf8b63ed296c: [clang codegen] Fix alignment of "Address" for incomplete array pointer. (authored by efriedma). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79052/new/ https://reviews.llvm.org/D79052 Files: clang/lib/CodeGen/CodeGenModule.cpp clang/test/CodeGenCXX/alignment.cpp Index: clang/test/CodeGenCXX/alignment.cpp === --- clang/test/CodeGenCXX/alignment.cpp +++ clang/test/CodeGenCXX/alignment.cpp @@ -308,4 +308,20 @@ D d; AlignedArray result = d.bArray; } + + // CHECK-LABEL: @_ZN5test11hEPA_NS_1BE + void h(B (*b)[]) { +// CHECK: [[RESULT:%.*]] = alloca [[ARRAY]], align 64 +// CHECK: [[B_P:%.*]] = load [0 x [[B]]]*, [0 x [[B]]]** +// CHECK: [[ELEMENT_P:%.*]] = getelementptr inbounds [0 x [[B]]], [0 x [[B]]]* [[B_P]], i64 0 +// CHECK: [[ARRAY_P:%.*]] = getelementptr inbounds [[B]], [[B]]* [[ELEMENT_P]], i32 0, i32 2 +// CHECK: [[T0:%.*]] = bitcast [[ARRAY]]* [[RESULT]] to i8* +// CHECK: [[T1:%.*]] = bitcast [[ARRAY]]* [[ARRAY_P]] to i8* +// CHECK: call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 64 [[T0]], i8* align 16 [[T1]], i64 16, i1 false) +AlignedArray result = (*b)->bArray; + } } + +// CHECK-LABEL: @_Z22incomplete_array_derefPA_i +// CHECK: load i32, i32* {{%.*}}, align 4 +int incomplete_array_deref(int (*p)[]) { return (*p)[2]; } Index: clang/lib/CodeGen/CodeGenModule.cpp === --- clang/lib/CodeGen/CodeGenModule.cpp +++ clang/lib/CodeGen/CodeGenModule.cpp @@ -5990,6 +5990,9 @@ if (TBAAInfo) *TBAAInfo = getTBAAAccessInfo(T); + // FIXME: This duplicates logic in ASTContext::getTypeAlignIfKnown. But + // that doesn't return the information we need to compute BaseInfo. + // Honor alignment typedef attributes even on incomplete types. // We also honor them straight for C++ class types, even as pointees; // there's an expressivity gap here. @@ -6001,32 +6004,46 @@ } } + bool AlignForArray = T->isArrayType(); + + // Analyze the base element type, so we don't get confused by incomplete + // array types. + T = getContext().getBaseElementType(T); + + if (T->isIncompleteType()) { +// We could try to replicate the logic from +// ASTContext::getTypeAlignIfKnown, but nothing uses the alignment if the +// type is incomplete, so it's impossible to test. We could try to reuse +// getTypeAlignIfKnown, but that doesn't return the information we need +// to set BaseInfo. So just ignore the possibility that the alignment is +// greater than one. +if (BaseInfo) + *BaseInfo = LValueBaseInfo(AlignmentSource::Type); +return CharUnits::One(); + } + if (BaseInfo) *BaseInfo = LValueBaseInfo(AlignmentSource::Type); CharUnits Alignment; - if (T->isIncompleteType()) { -Alignment = CharUnits::One(); // Shouldn't be used, but pessimistic is best. + // For C++ class pointees, we don't know whether we're pointing at a + // base or a complete object, so we generally need to use the + // non-virtual alignment. + const CXXRecordDecl *RD; + if (forPointeeType && !AlignForArray && (RD = T->getAsCXXRecordDecl())) { +Alignment = getClassPointerAlignment(RD); } else { -// For C++ class pointees, we don't know whether we're pointing at a -// base or a complete object, so we generally need to use the -// non-virtual alignment. -const CXXRecordDecl *RD; -if (forPointeeType && (RD = T->getAsCXXRecordDecl())) { - Alignment = getClassPointerAlignment(RD); -} else { - Alignment = getContext().getTypeAlignInChars(T); - if (T.getQualifiers().hasUnaligned()) -Alignment = CharUnits::One(); -} +Alignment = getContext().getTypeAlignInChars(T); +if (T.getQualifiers().hasUnaligned()) + Alignment = CharUnits::One(); + } -// Cap to the global maximum type alignment unless the alignment -// was somehow explicit on the type. -if (unsigned MaxAlign = getLangOpts().MaxTypeAlign) { - if (Alignment.getQuantity() > MaxAlign && - !getContext().isAlignmentRequired(T)) -Alignment = CharUnits::fromQuantity(MaxAlign); -} + // Cap to the global maximum type alignment unless the alignment + // was somehow explicit on the type. + if (unsigned MaxAlign = getLangOpts().MaxTypeAlign) { +if (Alignment.getQuantity() > MaxAlign && +!getContext().isAlignmentRequired(T)) + Alignment = CharUnits::fromQuantity(MaxAlign); } return Alignment; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D79052: [clang codegen] Fix alignment of "Address" for incomplete array pointer.
rsmith added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:176 CharUnits Alignment; - if (T->isIncompleteType()) { + if (T->getBaseElementTypeUnsafe()->isIncompleteType()) { Alignment = CharUnits::One(); // Shouldn't be used, but pessimistic is best. I don't know if it matters in practice, but this is still wrong. An incomplete type can have a known alignment, for a case like `struct alignas(32) S;`. Perhaps we should remove this test entirely and call `getTypeAlignIfKnown` instead of `getTypeAlign[InChars]` below. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79052/new/ https://reviews.llvm.org/D79052 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D79052: [clang codegen] Fix alignment of "Address" for incomplete array pointer.
efriedma created this revision. efriedma added a reviewer: rjmccall. Herald added a project: clang. rsmith added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:176 CharUnits Alignment; - if (T->isIncompleteType()) { + if (T->getBaseElementTypeUnsafe()->isIncompleteType()) { Alignment = CharUnits::One(); // Shouldn't be used, but pessimistic is best. I don't know if it matters in practice, but this is still wrong. An incomplete type can have a known alignment, for a case like `struct alignas(32) S;`. Perhaps we should remove this test entirely and call `getTypeAlignIfKnown` instead of `getTypeAlign[InChars]` below. The code was assuming all incomplete types don't have meaningful alignment, but that clearly isn't true. Fixes https://bugs.llvm.org/show_bug.cgi?id=45710 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D79052 Files: clang/lib/CodeGen/CodeGenFunction.cpp clang/test/CodeGenCXX/alignment.cpp Index: clang/test/CodeGenCXX/alignment.cpp === --- clang/test/CodeGenCXX/alignment.cpp +++ clang/test/CodeGenCXX/alignment.cpp @@ -309,3 +309,7 @@ AlignedArray result = d.bArray; } } + +// CHECK-LABEL: @_Z22incomplete_array_derefPA_i +// CHECK: load i32, i32* {{%.*}}, align 4 +int incomplete_array_deref(int (*p)[]) { return (*p)[2]; } Index: clang/lib/CodeGen/CodeGenFunction.cpp === --- clang/lib/CodeGen/CodeGenFunction.cpp +++ clang/lib/CodeGen/CodeGenFunction.cpp @@ -173,7 +173,7 @@ *BaseInfo = LValueBaseInfo(AlignmentSource::Type); CharUnits Alignment; - if (T->isIncompleteType()) { + if (T->getBaseElementTypeUnsafe()->isIncompleteType()) { Alignment = CharUnits::One(); // Shouldn't be used, but pessimistic is best. } else { // For C++ class pointees, we don't know whether we're pointing at a Index: clang/test/CodeGenCXX/alignment.cpp === --- clang/test/CodeGenCXX/alignment.cpp +++ clang/test/CodeGenCXX/alignment.cpp @@ -309,3 +309,7 @@ AlignedArray result = d.bArray; } } + +// CHECK-LABEL: @_Z22incomplete_array_derefPA_i +// CHECK: load i32, i32* {{%.*}}, align 4 +int incomplete_array_deref(int (*p)[]) { return (*p)[2]; } Index: clang/lib/CodeGen/CodeGenFunction.cpp === --- clang/lib/CodeGen/CodeGenFunction.cpp +++ clang/lib/CodeGen/CodeGenFunction.cpp @@ -173,7 +173,7 @@ *BaseInfo = LValueBaseInfo(AlignmentSource::Type); CharUnits Alignment; - if (T->isIncompleteType()) { + if (T->getBaseElementTypeUnsafe()->isIncompleteType()) { Alignment = CharUnits::One(); // Shouldn't be used, but pessimistic is best. } else { // For C++ class pointees, we don't know whether we're pointing at a ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D79052: [clang codegen] Fix alignment of "Address" for incomplete array pointer.
efriedma marked an inline comment as done. efriedma added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:176 CharUnits Alignment; - if (T->isIncompleteType()) { + if (T->getBaseElementTypeUnsafe()->isIncompleteType()) { Alignment = CharUnits::One(); // Shouldn't be used, but pessimistic is best. rsmith wrote: > I don't know if it matters in practice, but this is still wrong. An > incomplete type can have a known alignment, for a case like `struct > alignas(32) S;`. Perhaps we should remove this test entirely and call > `getTypeAlignIfKnown` instead of `getTypeAlign[InChars]` below. I can't think of any way to observe the alignment computed by getNaturalTypeAlignment for an incomplete class. We usually only use the alignment computed by getNaturalTypeAlignment() to set the alignment of memory operations, and you can't do any memory operations with an incomplete class. But the result might be easier to read, in any case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79052/new/ https://reviews.llvm.org/D79052 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D79052: [clang codegen] Fix alignment of "Address" for incomplete array pointer.
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:176 CharUnits Alignment; - if (T->isIncompleteType()) { + if (T->getBaseElementTypeUnsafe()->isIncompleteType()) { Alignment = CharUnits::One(); // Shouldn't be used, but pessimistic is best. efriedma wrote: > rsmith wrote: > > I don't know if it matters in practice, but this is still wrong. An > > incomplete type can have a known alignment, for a case like `struct > > alignas(32) S;`. Perhaps we should remove this test entirely and call > > `getTypeAlignIfKnown` instead of `getTypeAlign[InChars]` below. > I can't think of any way to observe the alignment computed by > getNaturalTypeAlignment for an incomplete class. We usually only use the > alignment computed by getNaturalTypeAlignment() to set the alignment of > memory operations, and you can't do any memory operations with an incomplete > class. > > But the result might be easier to read, in any case. If there's a `getTypeAlignIfKnown()`, it would be better to use it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79052/new/ https://reviews.llvm.org/D79052 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D79052: [clang codegen] Fix alignment of "Address" for incomplete array pointer.
efriedma updated this revision to Diff 267994. efriedma added a comment. "Address" the review comments. Not really happy with this, but not sure what else to do. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79052/new/ https://reviews.llvm.org/D79052 Files: clang/lib/CodeGen/CodeGenModule.cpp clang/test/CodeGenCXX/alignment.cpp Index: clang/test/CodeGenCXX/alignment.cpp === --- clang/test/CodeGenCXX/alignment.cpp +++ clang/test/CodeGenCXX/alignment.cpp @@ -309,3 +309,7 @@ AlignedArray result = d.bArray; } } + +// CHECK-LABEL: @_Z22incomplete_array_derefPA_i +// CHECK: load i32, i32* {{%.*}}, align 4 +int incomplete_array_deref(int (*p)[]) { return (*p)[2]; } Index: clang/lib/CodeGen/CodeGenModule.cpp === --- clang/lib/CodeGen/CodeGenModule.cpp +++ clang/lib/CodeGen/CodeGenModule.cpp @@ -5993,32 +5993,58 @@ } } + bool AlignForArray = T->isArrayType(); + + // Analyze the base element type, so we don't get confused by incomplete + // array types. + T = getBaseElementType(T); + + if (T->isIncompleteType()) { +// FIXME: This duplicates logic in ASTContext::getTypeAlignIfKnown. But we +// that doesn't return whether we got the alignment from an attribute. +if (const auto *TT = T->getAs()) { + if (unsigned Align = TT->getDecl()->getMaxAlignment()) { +if (BaseInfo) + *BaseInfo = LValueBaseInfo(AlignmentSource::AttributedType); +return getContext().toCharUnitsFromBits(Align); + } +} + +// Otherwise, see if the declaration of the type had an attribute. +if (const auto *TT = T->getAs()) { + if (BaseInfo) +*BaseInfo = LValueBaseInfo(AlignmentSource::AttributedType); + return getContext().toCharUnitsFromBits(TT->getDecl()->getMaxAlignment()); +} + +// The alignment is completely unknown; just return one. +if (BaseInfo) + *BaseInfo = LValueBaseInfo(AlignmentSource::Type); +return CharUnits::One(); + } + if (BaseInfo) *BaseInfo = LValueBaseInfo(AlignmentSource::Type); CharUnits Alignment; - if (T->isIncompleteType()) { -Alignment = CharUnits::One(); // Shouldn't be used, but pessimistic is best. + // For C++ class pointees, we don't know whether we're pointing at a + // base or a complete object, so we generally need to use the + // non-virtual alignment. + const CXXRecordDecl *RD; + if (forPointeeType && !AlignForArray && (RD = T->getAsCXXRecordDecl())) { +Alignment = getClassPointerAlignment(RD); } else { -// For C++ class pointees, we don't know whether we're pointing at a -// base or a complete object, so we generally need to use the -// non-virtual alignment. -const CXXRecordDecl *RD; -if (forPointeeType && (RD = T->getAsCXXRecordDecl())) { - Alignment = getClassPointerAlignment(RD); -} else { - Alignment = getContext().getTypeAlignInChars(T); - if (T.getQualifiers().hasUnaligned()) -Alignment = CharUnits::One(); -} +Alignment = getContext().getTypeAlignInChars(T); +if (T.getQualifiers().hasUnaligned()) + Alignment = CharUnits::One(); + } -// Cap to the global maximum type alignment unless the alignment -// was somehow explicit on the type. -if (unsigned MaxAlign = getLangOpts().MaxTypeAlign) { - if (Alignment.getQuantity() > MaxAlign && - !getContext().isAlignmentRequired(T)) -Alignment = CharUnits::fromQuantity(MaxAlign); -} + // Cap to the global maximum type alignment unless the alignment + // was somehow explicit on the type. + if (unsigned MaxAlign = getLangOpts().MaxTypeAlign) { +if (Alignment.getQuantity() > MaxAlign && +!getContext().isAlignmentRequired(T)) + Alignment = CharUnits::fromQuantity(MaxAlign); } return Alignment; } Index: clang/test/CodeGenCXX/alignment.cpp === --- clang/test/CodeGenCXX/alignment.cpp +++ clang/test/CodeGenCXX/alignment.cpp @@ -309,3 +309,7 @@ AlignedArray result = d.bArray; } } + +// CHECK-LABEL: @_Z22incomplete_array_derefPA_i +// CHECK: load i32, i32* {{%.*}}, align 4 +int incomplete_array_deref(int (*p)[]) { return (*p)[2]; } Index: clang/lib/CodeGen/CodeGenModule.cpp === --- clang/lib/CodeGen/CodeGenModule.cpp +++ clang/lib/CodeGen/CodeGenModule.cpp @@ -5993,32 +5993,58 @@ } } + bool AlignForArray = T->isArrayType(); + + // Analyze the base element type, so we don't get confused by incomplete + // array types. + T = getBaseElementType(T); + + if (T->isIncompleteType()) { +// FIXME: This duplicates logic in ASTContext::getTypeAlignIfKnown. But we +// that doesn't return whether we got the alignment from an attribute. +if (const auto *TT = T->getAs
[PATCH] D79052: [clang codegen] Fix alignment of "Address" for incomplete array pointer.
efriedma planned changes to this revision. efriedma added a comment. Hang on, I submitted this too early. Need to look a bit more. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79052/new/ https://reviews.llvm.org/D79052 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D79052: [clang codegen] Fix alignment of "Address" for incomplete array pointer.
efriedma updated this revision to Diff 267999. efriedma added a comment. This should work correctly, now, I think? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79052/new/ https://reviews.llvm.org/D79052 Files: clang/lib/CodeGen/CodeGenModule.cpp clang/test/CodeGenCXX/alignment.cpp Index: clang/test/CodeGenCXX/alignment.cpp === --- clang/test/CodeGenCXX/alignment.cpp +++ clang/test/CodeGenCXX/alignment.cpp @@ -308,4 +308,20 @@ D d; AlignedArray result = d.bArray; } + + // CHECK-LABEL: @_ZN5test11hEPA_NS_1BE + void h(B (*b)[]) { +// CHECK: [[RESULT:%.*]] = alloca [[ARRAY]], align 64 +// CHECK: [[B_P:%.*]] = load [0 x [[B]]]*, [0 x [[B]]]** +// CHECK: [[ELEMENT_P:%.*]] = getelementptr inbounds [0 x [[B]]], [0 x [[B]]]* [[B_P]], i64 0 +// CHECK: [[ARRAY_P:%.*]] = getelementptr inbounds [[B]], [[B]]* [[ELEMENT_P]], i32 0, i32 2 +// CHECK: [[T0:%.*]] = bitcast [[ARRAY]]* [[RESULT]] to i8* +// CHECK: [[T1:%.*]] = bitcast [[ARRAY]]* [[ARRAY_P]] to i8* +// CHECK: call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 64 [[T0]], i8* align 16 [[T1]], i64 16, i1 false) +AlignedArray result = (*b)->bArray; + } } + +// CHECK-LABEL: @_Z22incomplete_array_derefPA_i +// CHECK: load i32, i32* {{%.*}}, align 4 +int incomplete_array_deref(int (*p)[]) { return (*p)[2]; } Index: clang/lib/CodeGen/CodeGenModule.cpp === --- clang/lib/CodeGen/CodeGenModule.cpp +++ clang/lib/CodeGen/CodeGenModule.cpp @@ -5982,6 +5982,9 @@ if (TBAAInfo) *TBAAInfo = getTBAAAccessInfo(T); + // FIXME: This duplicates logic in ASTContext::getTypeAlignIfKnown. But + // that doesn't return the information we need to compute BaseInfo. + // Honor alignment typedef attributes even on incomplete types. // We also honor them straight for C++ class types, even as pointees; // there's an expressivity gap here. @@ -5993,32 +5996,46 @@ } } + bool AlignForArray = T->isArrayType(); + + // Analyze the base element type, so we don't get confused by incomplete + // array types. + T = getContext().getBaseElementType(T); + + if (T->isIncompleteType()) { +// We could try to replicate the logic from +// ASTContext::getTypeAlignIfKnown, but nothing uses the alignment if the +// type is incomplete, so it's impossible to test. We could try to reuse +// getTypeAlignIfKnown, but that doesn't return the information we need +// to set BaseInfo. So just ignore the possibility that the alignment is +// greater than one. +if (BaseInfo) + *BaseInfo = LValueBaseInfo(AlignmentSource::Type); +return CharUnits::One(); + } + if (BaseInfo) *BaseInfo = LValueBaseInfo(AlignmentSource::Type); CharUnits Alignment; - if (T->isIncompleteType()) { -Alignment = CharUnits::One(); // Shouldn't be used, but pessimistic is best. + // For C++ class pointees, we don't know whether we're pointing at a + // base or a complete object, so we generally need to use the + // non-virtual alignment. + const CXXRecordDecl *RD; + if (forPointeeType && !AlignForArray && (RD = T->getAsCXXRecordDecl())) { +Alignment = getClassPointerAlignment(RD); } else { -// For C++ class pointees, we don't know whether we're pointing at a -// base or a complete object, so we generally need to use the -// non-virtual alignment. -const CXXRecordDecl *RD; -if (forPointeeType && (RD = T->getAsCXXRecordDecl())) { - Alignment = getClassPointerAlignment(RD); -} else { - Alignment = getContext().getTypeAlignInChars(T); - if (T.getQualifiers().hasUnaligned()) -Alignment = CharUnits::One(); -} +Alignment = getContext().getTypeAlignInChars(T); +if (T.getQualifiers().hasUnaligned()) + Alignment = CharUnits::One(); + } -// Cap to the global maximum type alignment unless the alignment -// was somehow explicit on the type. -if (unsigned MaxAlign = getLangOpts().MaxTypeAlign) { - if (Alignment.getQuantity() > MaxAlign && - !getContext().isAlignmentRequired(T)) -Alignment = CharUnits::fromQuantity(MaxAlign); -} + // Cap to the global maximum type alignment unless the alignment + // was somehow explicit on the type. + if (unsigned MaxAlign = getLangOpts().MaxTypeAlign) { +if (Alignment.getQuantity() > MaxAlign && +!getContext().isAlignmentRequired(T)) + Alignment = CharUnits::fromQuantity(MaxAlign); } return Alignment; } ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D79052: [clang codegen] Fix alignment of "Address" for incomplete array pointer.
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79052/new/ https://reviews.llvm.org/D79052 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits