Skip to content

Commit a1ab6bf

Browse files
andykaylorandykaylor
authored
[CIR] Fix Address element type problems (#1373)
There were problems with the pointer type and element type of the Address class getting out of sync. In the traditional codegen the pointer has no type, so it was sufficient for the Address class to simply track the type it was supposed to be pointing to. Since ClangIR pointer values are typed, the Address::withType function wasn't really doing what it was supposed to. It returned an object with the same pointer that the original object had, but with a mismatched element type. This change updates the Address::withType function to perform a bitcast to get the expected pointer type before creating a new Address object. It also adds assertions in the Address class to verify that pointer type and element type are consistent and updates many places that were causing those assertions to fire. These code changes cause extra bitcasts to be emitted in a few places. Regression tests have been updated as needed to reflect the CIR that is now generated.
1 parent 5f68f6c commit a1ab6bf

19 files changed

+91
-60
lines changed

‎clang/lib/CIR/CodeGen/Address.h

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@
2525

2626
namespace clang::CIRGen {
2727

28+
// Forward declaration to avoid a circular dependency
29+
class CIRGenBuilderTy;
30+
2831
// Indicates whether a pointer is known not to be null.
2932
enum KnownNonNull_t { NotKnownNonNull, KnownNonNull };
3033

@@ -64,6 +67,9 @@ class Address {
6467
assert(pointer && "Pointer cannot be null");
6568
assert(elementType && "Element type cannot be null");
6669
assert(!alignment.isZero() && "Alignment cannot be zero");
70+
71+
assert(mlir::cast<cir::PointerType>(pointer.getType()).getPointee() ==
72+
ElementType);
6773
}
6874

6975
Address(mlir::Value basePtr, mlir::Type elementType,
@@ -104,15 +110,9 @@ class Address {
104110

105111
bool hasOffset() const { return bool(offset); }
106112

107-
/// Return address with different element type, but same pointer and
108-
/// alignment.
109-
Address withElementType(mlir::Type ElemTy) const {
110-
if (!hasOffset())
111-
return Address(getBasePointer(), ElemTy, getAlignment(),
112-
getPointerAuthInfo(), /*Offset=*/nullptr,
113-
isKnownNonNull());
114-
return Address(getPointer(), ElemTy, getAlignment(), isKnownNonNull());
115-
}
113+
/// Return address with different element type, a bitcast pointer, and
114+
/// the same alignment.
115+
Address withElementType(CIRGenBuilderTy &builder, mlir::Type ElemTy) const;
116116

117117
mlir::Value getPointer() const {
118118
assert(isValid());
@@ -142,11 +142,17 @@ class Address {
142142

143143
/// Return the type of the pointer value.
144144
cir::PointerType getType() const {
145+
assert(mlir::cast<cir::PointerType>(
146+
PointerAndKnownNonNull.getPointer().getType())
147+
.getPointee() == ElementType);
145148
return mlir::cast<cir::PointerType>(getPointer().getType());
146149
}
147150

148151
mlir::Type getElementType() const {
149152
assert(isValid());
153+
assert(mlir::cast<cir::PointerType>(
154+
PointerAndKnownNonNull.getPointer().getType())
155+
.getPointee() == ElementType);
150156
return ElementType;
151157
}
152158

‎clang/lib/CIR/CodeGen/CIRAsm.cpp

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -214,8 +214,9 @@ std::pair<mlir::Value, mlir::Type> CIRGenFunction::emitAsmInputLValue(
214214
getTargetHooks().isScalarizableAsmOperand(*this, Ty)) {
215215
Ty = cir::IntType::get(&getMLIRContext(), Size, false);
216216

217-
return {builder.createLoad(getLoc(Loc),
218-
InputValue.getAddress().withElementType(Ty)),
217+
return {builder.createLoad(
218+
getLoc(Loc),
219+
InputValue.getAddress().withElementType(builder, Ty)),
219220
mlir::Type()};
220221
}
221222
}
@@ -320,7 +321,7 @@ static void emitAsmStores(CIRGenFunction &CGF, const AsmStmt &S,
320321
// ResultTypeRequiresCast.size() elements of RegResults.
321322
if ((i < ResultTypeRequiresCast.size()) && ResultTypeRequiresCast[i]) {
322323
unsigned Size = CGF.getContext().getTypeSize(ResultRegQualTys[i]);
323-
Address A = Dest.getAddress().withElementType(ResultRegTypes[i]);
324+
Address A = Dest.getAddress().withElementType(Builder, ResultRegTypes[i]);
324325
if (CGF.getTargetHooks().isScalarizableAsmOperand(CGF, TruncTy)) {
325326
Builder.createStore(CGF.getLoc(S.getAsmLoc()), Tmp, A);
326327
continue;
@@ -478,7 +479,8 @@ mlir::LogicalResult CIRGenFunction::emitAsmStmt(const AsmStmt &S) {
478479
// Otherwise there will be a mis-match if the matrix is also an
479480
// input-argument which is represented as vector.
480481
if (isa<MatrixType>(OutExpr->getType().getCanonicalType()))
481-
DestAddr = DestAddr.withElementType(convertType(OutExpr->getType()));
482+
DestAddr =
483+
DestAddr.withElementType(builder, convertType(OutExpr->getType()));
482484

483485
ArgTypes.push_back(DestAddr.getType());
484486
ArgElemTypes.push_back(DestAddr.getElementType());

‎clang/lib/CIR/CodeGen/CIRGenAtomic.cpp

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,7 @@ Address AtomicInfo::castToAtomicIntPointer(Address addr) const {
305305
if (intTy && intTy.getWidth() == AtomicSizeInBits)
306306
return addr;
307307
auto ty = CGF.getBuilder().getUIntNTy(AtomicSizeInBits);
308-
return addr.withElementType(ty);
308+
return addr.withElementType(CGF.getBuilder(), ty);
309309
}
310310

311311
Address AtomicInfo::convertToAtomicIntPointer(Address Addr) const {
@@ -1243,8 +1243,9 @@ RValue CIRGenFunction::emitAtomicExpr(AtomicExpr *E) {
12431243
if (RValTy->isVoidType())
12441244
return RValue::get(nullptr);
12451245

1246-
return convertTempToRValue(Dest.withElementType(convertTypeForMem(RValTy)),
1247-
RValTy, E->getExprLoc());
1246+
return convertTempToRValue(
1247+
Dest.withElementType(builder, convertTypeForMem(RValTy)), RValTy,
1248+
E->getExprLoc());
12481249
}
12491250

12501251
// The memory order is not known at compile-time. The atomic operations
@@ -1321,8 +1322,10 @@ RValue CIRGenFunction::emitAtomicExpr(AtomicExpr *E) {
13211322

13221323
if (RValTy->isVoidType())
13231324
return RValue::get(nullptr);
1324-
return convertTempToRValue(Dest.withElementType(convertTypeForMem(RValTy)),
1325-
RValTy, E->getExprLoc());
1325+
1326+
return convertTempToRValue(
1327+
Dest.withElementType(builder, convertTypeForMem(RValTy)), RValTy,
1328+
E->getExprLoc());
13261329
}
13271330

13281331
void CIRGenFunction::emitAtomicStore(RValue rvalue, LValue lvalue,

‎clang/lib/CIR/CodeGen/CIRGenBuilder.cpp

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,4 +133,16 @@ uint64_t CIRGenBuilderTy::computeOffsetFromGlobalViewIndices(
133133
}
134134

135135
return offset;
136-
}
136+
}
137+
138+
// This can't be defined in Address.h because that file is included by
139+
// CIRGenBuilder.h
140+
Address Address::withElementType(CIRGenBuilderTy &builder,
141+
mlir::Type ElemTy) const {
142+
if (!hasOffset())
143+
return Address(builder.createPtrBitcast(getBasePointer(), ElemTy), ElemTy,
144+
getAlignment(), getPointerAuthInfo(), /*Offset=*/nullptr,
145+
isKnownNonNull());
146+
return Address(builder.createPtrBitcast(getPointer(), ElemTy), ElemTy,
147+
getAlignment(), isKnownNonNull());
148+
}

‎clang/lib/CIR/CodeGen/CIRGenBuilder.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -733,7 +733,7 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
733733
auto ptrTy = getPointerTo(destType);
734734
auto baseAddr = create<cir::BaseClassAddrOp>(
735735
loc, ptrTy, addr.getPointer(), mlir::APInt(64, offset), assumeNotNull);
736-
return Address(baseAddr, ptrTy, addr.getAlignment());
736+
return Address(baseAddr, destType, addr.getAlignment());
737737
}
738738

739739
Address createDerivedClassAddr(mlir::Location loc, Address addr,
@@ -745,7 +745,7 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
745745
auto ptrTy = getPointerTo(destType);
746746
auto derivedAddr = create<cir::DerivedClassAddrOp>(
747747
loc, ptrTy, addr.getPointer(), mlir::APInt(64, offset), assumeNotNull);
748-
return Address(derivedAddr, ptrTy, addr.getAlignment());
748+
return Address(derivedAddr, destType, addr.getAlignment());
749749
}
750750

751751
mlir::Value createVTTAddrPoint(mlir::Location loc, mlir::Type retTy,

‎clang/lib/CIR/CodeGen/CIRGenBuiltinAArch64.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4502,7 +4502,7 @@ CIRGenFunction::emitAArch64BuiltinExpr(unsigned BuiltinID, const CallExpr *E,
45024502
}
45034503
case NEON::BI__builtin_neon_vld1_dup_v:
45044504
case NEON::BI__builtin_neon_vld1q_dup_v: {
4505-
Address ptrAddr = PtrOp0.withElementType(vTy.getEltType());
4505+
Address ptrAddr = PtrOp0.withElementType(builder, vTy.getEltType());
45064506
mlir::Value val = builder.createLoad(getLoc(E->getExprLoc()), ptrAddr);
45074507
cir::VecSplatOp vecSplat =
45084508
builder.create<cir::VecSplatOp>(getLoc(E->getExprLoc()), vTy, val);

‎clang/lib/CIR/CodeGen/CIRGenCXX.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -408,8 +408,7 @@ void CIRGenModule::emitCXXGlobalVarDeclInit(const VarDecl *varDecl,
408408
builder.setInsertionPointToStart(block);
409409
auto getGlobal = builder.createGetGlobal(addr);
410410

411-
Address declAddr(getGlobal, getGlobal.getType(),
412-
getASTContext().getDeclAlign(varDecl));
411+
Address declAddr(getGlobal, getASTContext().getDeclAlign(varDecl));
413412
assert(performInit && "cannot have constant initializer which needs "
414413
"destruction for reference");
415414
RValue rv = cgf.emitReferenceBindingToExpr(init);

‎clang/lib/CIR/CodeGen/CIRGenClass.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1672,7 +1672,7 @@ CIRGenFunction::getAddressOfBaseClass(Address Value,
16721672
VBase, BaseValueTy, not NullCheckValue);
16731673

16741674
// Cast to the destination type.
1675-
Value = Value.withElementType(BaseValueTy);
1675+
Value = Value.withElementType(builder, BaseValueTy);
16761676

16771677
return Value;
16781678
}
@@ -1894,7 +1894,7 @@ void CIRGenFunction::emitCXXAggrConstructorCall(
18941894
builder.create<cir::ArrayCtor>(
18951895
*currSrcLoc, arrayOp, [&](mlir::OpBuilder &b, mlir::Location loc) {
18961896
auto arg = b.getInsertionBlock()->addArgument(ptrToElmType, loc);
1897-
Address curAddr = Address(arg, ptrToElmType, eltAlignment);
1897+
Address curAddr = Address(arg, elementType, eltAlignment);
18981898
auto currAVS = AggValueSlot::forAddr(
18991899
curAddr, type.getQualifiers(), AggValueSlot::IsDestructed,
19001900
AggValueSlot::DoesNotNeedGCBarriers, AggValueSlot::IsNotAliased,

‎clang/lib/CIR/CodeGen/CIRGenDecl.cpp

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -244,11 +244,8 @@ static void emitStoresForConstant(CIRGenModule &CGM, const VarDecl &D,
244244
// FIXME(cir): This is closer to memcpy behavior but less optimal, instead of
245245
// copy from a global, we just create a cir.const out of it.
246246

247-
if (addr.getElementType() != Ty) {
248-
auto ptr = addr.getPointer();
249-
ptr = builder.createBitcast(ptr.getLoc(), ptr, builder.getPointerTo(Ty));
250-
addr = addr.withPointer(ptr, addr.isKnownNonNull());
251-
}
247+
if (addr.getElementType() != Ty)
248+
addr = addr.withElementType(builder, Ty);
252249

253250
auto loc = CGM.getLoc(D.getSourceRange());
254251
builder.createStore(loc, builder.getConstant(loc, constant), addr);
@@ -1108,7 +1105,7 @@ void CIRGenFunction::emitArrayDestroy(mlir::Value begin, mlir::Value end,
11081105
builder.create<cir::ArrayDtor>(
11091106
*currSrcLoc, begin, [&](mlir::OpBuilder &b, mlir::Location loc) {
11101107
auto arg = b.getInsertionBlock()->addArgument(ptrToElmType, loc);
1111-
Address curAddr = Address(arg, ptrToElmType, elementAlign);
1108+
Address curAddr = Address(arg, cirElementType, elementAlign);
11121109
if (useEHCleanup) {
11131110
pushRegularPartialArrayCleanup(arg, arg, elementType, elementAlign,
11141111
destroyer);

‎clang/lib/CIR/CodeGen/CIRGenException.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ void CIRGenFunction::emitAnyExprToExn(const Expr *e, Address addr) {
237237
// __cxa_allocate_exception returns a void*; we need to cast this
238238
// to the appropriate type for the object.
239239
auto ty = convertTypeForMem(e->getType());
240-
Address typedAddr = addr.withElementType(ty);
240+
Address typedAddr = addr.withElementType(builder, ty);
241241

242242
// From LLVM's codegen:
243243
// FIXME: this isn't quite right! If there's a final unelided call

0 commit comments

Comments
 (0)