Skip to content

Commit 9e10f04

Browse files
AmrDeveloperAmrDeveloper
committed
Address code review comments
1 parent d479f0d commit 9e10f04

File tree

10 files changed

+73
-47
lines changed

10 files changed

+73
-47
lines changed

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

+11-10
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,8 @@ void AggExprEmitter::emitArrayInit(Address destPtr, cir::ArrayType arrayTy,
159159

160160
// TODO:Replace this part later with cir::DoWhileOp
161161
for (unsigned i = numInitElements; i != numArrayElements; ++i) {
162-
auto currentElement = builder.createLoad(loc, tmpAddr.getPointer());
162+
cir::LoadOp currentElement =
163+
builder.createLoad(loc, tmpAddr.getPointer());
163164

164165
// Emit the actual filler expression.
165166
const LValue elementLV = LValue::makeAddr(
@@ -183,22 +184,21 @@ void AggExprEmitter::emitInitializationToLValue(Expr *e, LValue lv) {
183184
const QualType type = lv.getType();
184185

185186
if (isa<ImplicitValueInitExpr, CXXScalarValueInitExpr>(e)) {
186-
const auto loc = e->getSourceRange().isValid()
187-
? cgf.getLoc(e->getSourceRange())
188-
: *cgf.currSrcLoc;
187+
const mlir::Location loc = e->getSourceRange().isValid()
188+
? cgf.getLoc(e->getSourceRange())
189+
: *cgf.currSrcLoc;
189190
return emitNullInitializationToLValue(loc, lv);
190191
}
191192

192193
if (isa<NoInitExpr>(e))
193194
return;
194195

195-
if (type->isReferenceType()) {
196-
llvm_unreachable("NTI");
197-
}
196+
if (type->isReferenceType())
197+
cgf.cgm.errorNYI("emitInitializationToLValue ReferenceType");
198198

199199
switch (cgf.getEvaluationKind(type)) {
200200
case cir::TEK_Complex:
201-
llvm_unreachable("TEK_Complex NYI");
201+
cgf.cgm.errorNYI("emitInitializationToLValue TEK_Complex");
202202
break;
203203
case cir::TEK_Aggregate:
204204
cgf.emitAggExpr(e, AggValueSlot::forLValue(lv));
@@ -229,7 +229,7 @@ void AggExprEmitter::emitNullInitializationToLValue(mlir::Location loc,
229229
return;
230230
}
231231

232-
llvm_unreachable("emitStoreThroughBitfieldLValue NYI");
232+
cgf.cgm.errorNYI("emitStoreThroughBitfieldLValue");
233233
return;
234234
}
235235

@@ -265,7 +265,8 @@ void AggExprEmitter::visitCXXParenListOrInitListExpr(
265265
return;
266266
}
267267

268-
llvm_unreachable("NYI");
268+
cgf.cgm.errorNYI(
269+
"visitCXXParenListOrInitListExpr Record or VariableSizeArray type");
269270
}
270271

271272
void CIRGenFunction::emitAggExpr(const Expr *e, AggValueSlot slot) {

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

+5-6
Original file line numberDiff line numberDiff line change
@@ -421,17 +421,16 @@ mlir::Value CIRGenModule::emitNullConstant(QualType t, mlir::Location loc) {
421421
if (getTypes().isZeroInitializable(t))
422422
return builder.getNullValue(getTypes().convertTypeForMem(t), loc);
423423

424-
if (const ConstantArrayType *cat =
425-
getASTContext().getAsConstantArrayType(t)) {
426-
llvm_unreachable("NYI");
424+
if (getASTContext().getAsConstantArrayType(t)) {
425+
errorNYI("CIRGenModule::emitNullConstant ConstantArrayType");
427426
}
428427

429-
if (const RecordType *rt = t->getAs<RecordType>())
430-
llvm_unreachable("NYI");
428+
if (t->getAs<RecordType>())
429+
errorNYI("CIRGenModule::emitNullConstant RecordType");
431430

432431
assert(t->isMemberDataPointerType() &&
433432
"Should only see pointers to data members here!");
434433

435-
llvm_unreachable("NYI");
434+
errorNYI("CIRGenModule::emitNullConstant unsupported type");
436435
return {};
437436
}

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -461,7 +461,7 @@ void CIRGenFunction::emitNullInitialization(mlir::Location loc, Address destPtr,
461461

462462
// Cast the dest ptr to the appropriate i8 pointer type.
463463
if (builder.isInt8Ty(destPtr.getElementType())) {
464-
llvm_unreachable("NYI");
464+
cgm.errorNYI(loc, "Cast the dest ptr to the appropriate i8 pointer type");
465465
}
466466

467467
// Get size and alignment info for this aggregate.
@@ -489,7 +489,7 @@ void CIRGenFunction::emitNullInitialization(mlir::Location loc, Address destPtr,
489489
// Builder.CreateMemSet. In CIR just emit a store of #cir.zero to the
490490
// respective address.
491491
// Builder.CreateMemSet(DestPtr, Builder.getInt8(0), SizeVal, false);
492-
auto zeroValue = builder.getNullValue(convertType(ty), loc);
492+
const mlir::Value zeroValue = builder.getNullValue(convertType(ty), loc);
493493
builder.createStore(loc, zeroValue, destPtr.getPointer());
494494
}
495495

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

+2-3
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,8 @@ class CIRGenModule : public CIRGenTypeCache {
113113
void emitGlobalVarDefinition(const clang::VarDecl *vd,
114114
bool isTentative = false);
115115

116-
// Return the result of value-initializing the given type, i.e. a null
117-
/// expression of the given type. This is usually, but not always, an LLVM
118-
/// null constant.
116+
/// Return the result of value-initializing the given type, i.e. a null
117+
/// expression of the given type.
119118
mlir::Value emitNullConstant(QualType t, mlir::Location loc);
120119

121120
cir::FuncOp

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

+5-2
Original file line numberDiff line numberDiff line change
@@ -268,13 +268,16 @@ bool CIRGenTypes::isZeroInitializable(clang::QualType t) {
268268
return true;
269269
}
270270

271-
if (const RecordType *rt = t->getAs<RecordType>()) {
271+
if (t->getAs<RecordType>()) {
272272
cgm.errorNYI(SourceLocation(), "isZeroInitializable for RecordType", t);
273+
return false;
273274
}
274275

275-
if (const MemberPointerType *mpt = t->getAs<MemberPointerType>())
276+
if (t->getAs<MemberPointerType>()) {
276277
cgm.errorNYI(SourceLocation(), "isZeroInitializable for MemberPointerType",
277278
t);
279+
return false;
280+
}
278281

279282
return true;
280283
}

‎clang/lib/CIR/Dialect/IR/CIRDialect.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -246,8 +246,8 @@ OpFoldResult cir::ConstantOp::fold(FoldAdaptor /*adaptor*/) {
246246
//===----------------------------------------------------------------------===//
247247

248248
LogicalResult cir::CastOp::verify() {
249-
const auto resType = getResult().getType();
250-
const auto srcType = getSrc().getType();
249+
const mlir::Type resType = getResult().getType();
250+
const mlir::Type srcType = getSrc().getType();
251251

252252
switch (getKind()) {
253253
case cir::CastKind::int_to_bool: {

‎clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp

+7-9
Original file line numberDiff line numberDiff line change
@@ -546,8 +546,6 @@ mlir::LogicalResult CIRToLLVMPtrStrideOpLowering::matchAndRewrite(
546546
cir::PtrStrideOp ptrStrideOp, OpAdaptor adaptor,
547547
mlir::ConversionPatternRewriter &rewriter) const {
548548

549-
const mlir::DataLayout llvmLayout(
550-
ptrStrideOp->getParentOfType<mlir::ModuleOp>());
551549
const mlir::TypeConverter *tc = getTypeConverter();
552550
const mlir::Type resultTy = tc->convertType(ptrStrideOp.getType());
553551

@@ -566,9 +564,9 @@ mlir::LogicalResult CIRToLLVMPtrStrideOpLowering::matchAndRewrite(
566564
const unsigned width =
567565
mlir::cast<mlir::IntegerType>(index.getType()).getWidth();
568566
const std::optional<std::uint64_t> layoutWidth =
569-
llvmLayout.getTypeIndexBitwidth(adaptor.getBase().getType());
567+
dataLayout.getTypeIndexBitwidth(adaptor.getBase().getType());
570568

571-
const auto indexOp = index.getDefiningOp();
569+
mlir::Operation *indexOp = index.getDefiningOp();
572570
if (indexOp && layoutWidth && width != *layoutWidth) {
573571
// If the index comes from a subtraction, make sure the extension happens
574572
// before it. To achieve that, look at unary minus, which already got
@@ -691,7 +689,7 @@ mlir::Value lowerCirAttrAsValue(mlir::Operation *parentOp,
691689
const mlir::TypeConverter *converter,
692690
mlir::DataLayout const &dataLayout) {
693691
CIRAttrToValue valueConverter(parentOp, rewriter, converter);
694-
auto value = valueConverter.visit(attr);
692+
const mlir::Value value = valueConverter.visit(attr);
695693
if (!value)
696694
llvm_unreachable("unhandled attribute type");
697695
return value;
@@ -751,16 +749,16 @@ mlir::LogicalResult CIRToLLVMConstantOpLowering::matchAndRewrite(
751749

752750
std::optional<mlir::Attribute> denseAttr;
753751
if (constArr && hasTrailingZeros(constArr)) {
754-
auto newOp = lowerCirAttrAsValue(op, constArr, rewriter,
755-
getTypeConverter(), dataLayout);
752+
const mlir::Value newOp = lowerCirAttrAsValue(
753+
op, constArr, rewriter, getTypeConverter(), dataLayout);
756754
rewriter.replaceOp(op, newOp);
757755
return mlir::success();
758756
} else if (constArr &&
759757
(denseAttr = lowerConstArrayAttr(constArr, typeConverter))) {
760758
attr = denseAttr.value();
761759
} else {
762-
auto initVal = lowerCirAttrAsValue(op, op.getValue(), rewriter,
763-
typeConverter, dataLayout);
760+
const mlir::Value initVal = lowerCirAttrAsValue(
761+
op, op.getValue(), rewriter, typeConverter, dataLayout);
764762
rewriter.replaceAllUsesWith(op, initVal);
765763
rewriter.eraseOp(op);
766764
return mlir::success();

‎clang/lib/CIR/Lowering/LoweringHelpers.cpp

+3-5
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,12 @@ mlir::DenseElementsAttr
1616
convertStringAttrToDenseElementsAttr(cir::ConstArrayAttr attr,
1717
mlir::Type type) {
1818
auto values = llvm::SmallVector<mlir::APInt, 8>{};
19-
const auto stringAttr = mlir::dyn_cast<mlir::StringAttr>(attr.getElts());
20-
assert(stringAttr && "expected string attribute here");
19+
const auto stringAttr = mlir::cast<mlir::StringAttr>(attr.getElts());
2120

22-
for (auto element : stringAttr)
21+
for (const char element : stringAttr)
2322
values.push_back({8, (uint64_t)element});
2423

25-
const auto arrayTy = mlir::dyn_cast<cir::ArrayType>(attr.getType());
26-
assert(arrayTy && "String attribute must have an array type");
24+
const auto arrayTy = mlir::cast<cir::ArrayType>(attr.getType());
2725
if (arrayTy.getSize() != stringAttr.size())
2826
llvm_unreachable("array type of the length not equal to that of the string "
2927
"attribute is not supported yet");

‎clang/test/CIR/CodeGen/array.cpp

+19-4
Original file line numberDiff line numberDiff line change
@@ -117,10 +117,25 @@ void func6() {
117117
// CHECK: cir.store %[[V1]], %[[ELE_PTR]] : !s32i, !cir.ptr<!s32i>
118118
}
119119

120-
void func7(int p[10]) {}
121-
// CHECK: cir.func @func7(%arg0: !cir.ptr<!s32i>
120+
void func7() {
121+
int* arr[1] = {};
122+
123+
// CHECK: %[[ARR:.*]] = cir.alloca !cir.array<!cir.ptr<!s32i> x 1>, !cir.ptr<!cir.array<!cir.ptr<!s32i> x 1>>, ["arr", init]
124+
// CHECK: %[[ARR_TMP:.*]] = cir.alloca !cir.ptr<!cir.ptr<!s32i>>, !cir.ptr<!cir.ptr<!cir.ptr<!s32i>>>, ["arrayinit.temp", init]
125+
// CHECK: %[[ARR_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR]] : !cir.ptr<!cir.array<!cir.ptr<!s32i> x 1>>), !cir.ptr<!cir.ptr<!s32i>>
126+
// CHECK: cir.store %[[ARR_PTR]], %[[ARR_TMP]] : !cir.ptr<!cir.ptr<!s32i>>, !cir.ptr<!cir.ptr<!cir.ptr<!s32i>>>
127+
// CHECK: %[[TMP:.*]] = cir.load %[[ARR_TMP]] : !cir.ptr<!cir.ptr<!cir.ptr<!s32i>>>, !cir.ptr<!cir.ptr<!s32i>>
128+
// CHECK: %[[NULL_PTR:.*]] = cir.const #cir.ptr<null> : !cir.ptr<!s32i>
129+
// CHECK: cir.store %[[NULL_PTR]], %[[TMP]] : !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>>
130+
// CHECK: %[[OFFSET:.*]] = cir.const #cir.int<1> : !s64i
131+
// CHECK: %[[ELE_PTR:.*]] = cir.ptr_stride(%[[TMP]] : !cir.ptr<!cir.ptr<!s32i>>, %[[OFFSET]] : !s64i), !cir.ptr<!cir.ptr<!s32i>>
132+
// CHECK: cir.store %[[ELE_PTR]], %[[ARR_TMP]] : !cir.ptr<!cir.ptr<!s32i>>, !cir.ptr<!cir.ptr<!cir.ptr<!s32i>>>
133+
}
134+
135+
void func8(int p[10]) {}
136+
// CHECK: cir.func @func8(%arg0: !cir.ptr<!s32i>
122137
// CHECK: cir.alloca !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>>, ["p", init]
123138

124-
void func8(int pp[10][5]) {}
125-
// CHECK: cir.func @func8(%arg0: !cir.ptr<!cir.array<!s32i x 5>>
139+
void func9(int pp[10][5]) {}
140+
// CHECK: cir.func @func9(%arg0: !cir.ptr<!cir.array<!s32i x 5>>
126141
// CHECK: cir.alloca !cir.ptr<!cir.array<!s32i x 5>>, !cir.ptr<!cir.ptr<!cir.array<!s32i x 5>>>

‎clang/test/CIR/Lowering/array.cpp

+17-4
Original file line numberDiff line numberDiff line change
@@ -102,10 +102,23 @@ void func6() {
102102
// CHECK: %[[ELE_1:.*]] = getelementptr i32, ptr %[[ELE_0]], i64 1
103103
// CHECK: store i32 5, ptr %[[ELE_1]], align 4
104104

105-
void func7(int p[10]) {}
106-
// CHECK: define void @func7(ptr {{%.*}})
105+
void func7() {
106+
int* arr[1] = {};
107+
}
108+
// CHECK: define void @func7()
109+
// CHECK: %[[ARR:.*]] = alloca [1 x ptr], i64 1, align 8
110+
// CHECK: %[[ALLOCA:.*]] = alloca ptr, i64 1, align 8
111+
// CHECK: %[[ELE_PTR:.*]] = getelementptr ptr, ptr %[[ARR]], i32 0
112+
// CHECK: store ptr %[[ELE_PTR]], ptr %[[ALLOCA]], align 8
113+
// CHECK: %[[TMP:.*]] = load ptr, ptr %[[ALLOCA]], align 8
114+
// CHECK: store ptr null, ptr %[[TMP]], align 8
115+
// CHECK: %[[ELE:.*]] = getelementptr ptr, ptr %[[TMP]], i64 1
116+
// CHECK: store ptr %[[ELE]], ptr %[[ALLOCA]], align 8
117+
118+
void func8(int p[10]) {}
119+
// CHECK: define void @func8(ptr {{%.*}})
107120
// CHECK-NEXT: alloca ptr, i64 1, align 8
108121

109-
void func8(int pp[10][5]) {}
110-
// CHECK: define void @func8(ptr {{%.*}})
122+
void func9(int pp[10][5]) {}
123+
// CHECK: define void @func9(ptr {{%.*}})
111124
// CHECK-NEXT: alloca ptr, i64 1, align 8

0 commit comments

Comments
 (0)