Skip to content

Commit e7d093b

Browse files
AmrDeveloperAmrDeveloper
committed
Address code review comments
1 parent 8d2acb1 commit e7d093b

File tree

5 files changed

+54
-47
lines changed

5 files changed

+54
-47
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -200,15 +200,15 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
200200
}
201201

202202
/// Create a cir.ptr_stride operation to get access to an array element.
203-
/// idx is the index of the element to access, shouldDecay is true if the
204-
/// result should decay to a pointer to the element type.
203+
/// \p idx is the index of the element to access, \p shouldDecay is true if
204+
/// the result should decay to a pointer to the element type.
205205
mlir::Value getArrayElement(mlir::Location arrayLocBegin,
206206
mlir::Location arrayLocEnd, mlir::Value arrayPtr,
207207
mlir::Type eltTy, mlir::Value idx,
208208
bool shouldDecay);
209209

210210
/// Returns a decayed pointer to the first element of the array
211-
/// pointed to by arrayPtr.
211+
/// pointed to by \p arrayPtr.
212212
mlir::Value maybeBuildArrayDecay(mlir::Location loc, mlir::Value arrayPtr,
213213
mlir::Type eltTy);
214214
};

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

Lines changed: 28 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ LValue CIRGenFunction::emitUnaryOpLValue(const UnaryOperator *e) {
236236
/// If the specified expr is a simple decay from an array to pointer,
237237
/// return the array subexpression.
238238
/// FIXME: this could be abstracted into a common AST helper.
239-
static const Expr *isSimpleArrayDecayOperand(const Expr *e) {
239+
static const Expr *getSimpleArrayDecayOperand(const Expr *e) {
240240
// If this isn't just an array->pointer decay, bail out.
241241
const auto *castExpr = dyn_cast<CastExpr>(e);
242242
if (!castExpr || castExpr->getCastKind() != CK_ArrayToPointerDecay)
@@ -279,18 +279,12 @@ static QualType getFixedSizeElementType(const ASTContext &astContext,
279279
return eltType;
280280
}
281281

282-
static mlir::Value
283-
emitArraySubscriptPtr(CIRGenFunction &cgf, mlir::Location beginLoc,
284-
mlir::Location endLoc, mlir::Value ptr, mlir::Type eltTy,
285-
ArrayRef<mlir::Value> indices, bool inbounds,
286-
bool signedIndices, bool shouldDecay,
287-
const llvm::Twine &name = "arrayidx") {
288-
if (indices.size() > 1) {
289-
cgf.cgm.errorNYI("emitArraySubscriptPtr: handle multiple indices");
290-
return {};
291-
}
292-
293-
const mlir::Value idx = indices.back();
282+
static mlir::Value emitArraySubscriptPtr(CIRGenFunction &cgf,
283+
mlir::Location beginLoc,
284+
mlir::Location endLoc, mlir::Value ptr,
285+
mlir::Type eltTy, mlir::Value idx,
286+
bool inbounds, bool signedIndices,
287+
bool shouldDecay) {
294288
CIRGenModule &cgm = cgf.getCIRGenModule();
295289
// TODO(cir): LLVM codegen emits in bound gep check here, is there anything
296290
// that would enhance tracking this later in CIR?
@@ -300,12 +294,11 @@ emitArraySubscriptPtr(CIRGenFunction &cgf, mlir::Location beginLoc,
300294
shouldDecay);
301295
}
302296

303-
static Address emitArraySubscriptPtr(
304-
CIRGenFunction &cgf, mlir::Location beginLoc, mlir::Location endLoc,
305-
Address addr, ArrayRef<mlir::Value> indices, QualType eltType,
306-
bool inbounds, bool signedIndices, mlir::Location loc, bool shouldDecay,
307-
QualType *arrayType = nullptr, const Expr *base = nullptr,
308-
const llvm::Twine &name = "arrayidx") {
297+
static Address
298+
emitArraySubscriptPtr(CIRGenFunction &cgf, mlir::Location beginLoc,
299+
mlir::Location endLoc, Address addr, QualType eltType,
300+
mlir::Value idx, bool inbounds, mlir::Location loc,
301+
bool shouldDecay, QualType *arrayType, const Expr *base) {
309302

310303
// Determine the element size of the statically-sized base. This is
311304
// the thing that the indices are expressed in terms of.
@@ -317,14 +310,16 @@ static Address emitArraySubscriptPtr(
317310
// We can use that to compute the best alignment of the element.
318311
const CharUnits eltSize = cgf.getContext().getTypeSizeInChars(eltType);
319312
const CharUnits eltAlign =
320-
getArrayElementAlign(addr.getAlignment(), indices.back(), eltSize);
313+
getArrayElementAlign(addr.getAlignment(), idx, eltSize);
321314

322315
mlir::Value eltPtr;
323-
const mlir::IntegerAttr lastIndex = getConstantIndexOrNull(indices.back());
324-
if (!lastIndex) {
325-
eltPtr = emitArraySubscriptPtr(cgf, beginLoc, endLoc, addr.getPointer(),
326-
addr.getElementType(), indices, inbounds,
327-
signedIndices, shouldDecay, name);
316+
const mlir::IntegerAttr index = getConstantIndexOrNull(idx);
317+
if (!index) {
318+
eltPtr = emitArraySubscriptPtr(
319+
cgf, beginLoc, endLoc, addr.getPointer(), addr.getElementType(), idx,
320+
inbounds, idx.getType().isSignlessIntOrIndex(), shouldDecay);
321+
} else {
322+
cgf.cgm.errorNYI("emitArraySubscriptExpr: Non Constant Index");
328323
}
329324
const mlir::Type elementType = cgf.convertTypeForMem(eltType);
330325
return Address(eltPtr, elementType, eltAlign);
@@ -335,35 +330,32 @@ CIRGenFunction::emitArraySubscriptExpr(const clang::ArraySubscriptExpr *e) {
335330
if (e->getBase()->getType()->isVectorType() &&
336331
!isa<ExtVectorElementExpr>(e->getBase())) {
337332
cgm.errorNYI(e->getSourceRange(), "emitArraySubscriptExpr: VectorType");
338-
return {};
333+
return LValue::makeAddr(Address::invalid(), e->getType());
339334
}
340335

341336
if (isa<ExtVectorElementExpr>(e->getBase())) {
342337
cgm.errorNYI(e->getSourceRange(),
343338
"emitArraySubscriptExpr: ExtVectorElementExpr");
344-
return {};
339+
return LValue::makeAddr(Address::invalid(), e->getType());
345340
}
346341

347342
if (getContext().getAsVariableArrayType(e->getType())) {
348343
cgm.errorNYI(e->getSourceRange(),
349344
"emitArraySubscriptExpr: VariableArrayType");
350-
return {};
345+
return LValue::makeAddr(Address::invalid(), e->getType());
351346
}
352347

353348
if (e->getType()->getAs<ObjCObjectType>()) {
354349
cgm.errorNYI(e->getSourceRange(), "emitArraySubscriptExpr: ObjCObjectType");
355-
return {};
350+
return LValue::makeAddr(Address::invalid(), e->getType());
356351
}
357352

358353
// The index must always be an integer, which is not an aggregate. Emit it
359354
// in lexical order (this complexity is, sadly, required by C++17).
360355
assert((e->getIdx() == e->getLHS() || e->getIdx() == e->getRHS()) &&
361356
"index was neither LHS nor RHS");
362357
const mlir::Value idx = emitScalarExpr(e->getIdx());
363-
const QualType idxTy = e->getIdx()->getType();
364-
const bool signedIndices = idxTy->isSignedIntegerOrEnumerationType();
365-
366-
if (const Expr *array = isSimpleArrayDecayOperand(e->getBase())) {
358+
if (const Expr *array = getSimpleArrayDecayOperand(e->getBase())) {
367359
LValue arrayLV;
368360
if (const auto *ase = dyn_cast<ArraySubscriptExpr>(array))
369361
arrayLV = emitArraySubscriptExpr(ase);
@@ -374,10 +366,9 @@ CIRGenFunction::emitArraySubscriptExpr(const clang::ArraySubscriptExpr *e) {
374366
QualType arrayType = array->getType();
375367
const Address addr = emitArraySubscriptPtr(
376368
*this, cgm.getLoc(array->getBeginLoc()), cgm.getLoc(array->getEndLoc()),
377-
arrayLV.getAddress(), {idx}, e->getType(),
378-
!getLangOpts().isSignedOverflowDefined(), signedIndices,
379-
cgm.getLoc(e->getExprLoc()), /*shouldDecay=*/true, &arrayType,
380-
e->getBase());
369+
arrayLV.getAddress(), e->getType(), idx,
370+
!getLangOpts().isSignedOverflowDefined(), cgm.getLoc(e->getExprLoc()),
371+
/*shouldDecay=*/true, &arrayType, e->getBase());
381372

382373
return LValue::makeAddr(addr, e->getType());
383374
}

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,8 +159,9 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
159159

160160
mlir::Value VisitArraySubscriptExpr(ArraySubscriptExpr *e) {
161161
if (e->getBase()->getType()->isVectorType()) {
162-
assert(!cir::MissingFeatures::scalableVectors() &&
163-
"NYI: index into scalable vector");
162+
assert(!cir::MissingFeatures::scalableVectors());
163+
cgf.getCIRGenModule().errorNYI("VisitArraySubscriptExpr: VectorType");
164+
return {};
164165
}
165166
// Just load the lvalue formed by the subscript expression.
166167
return emitLoadOfLValue(e);

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

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,15 +29,24 @@ int f[5] = {1, 2};
2929

3030
void func() {
3131
int arr[10];
32-
// CHECK: %[[ARR:.*]] = cir.alloca !cir.array<!s32i x 10>, !cir.ptr<!cir.array<!s32i x 10>>, ["arr"]
32+
int e = arr[0];
33+
int e2 = arr[1];
3334

34-
int e = arr[1];
35+
// CHECK: %[[ARR:.*]] = cir.alloca !cir.array<!s32i x 10>, !cir.ptr<!cir.array<!s32i x 10>>, ["arr"]
3536
// CHECK: %[[INIT:.*]] = cir.alloca !s32i, !cir.ptr<!s32i>, ["e", init]
36-
// CHECK: %[[IDX:.*]] = cir.const #cir.int<1> : !s32i
37+
// CHECK: %[[INIT_2:.*]] = cir.alloca !s32i, !cir.ptr<!s32i>, ["e2", init]
38+
39+
// CHECK: %[[IDX:.*]] = cir.const #cir.int<0> : !s32i
3740
// CHECK: %[[ARR_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR]] : !cir.ptr<!cir.array<!s32i x 10>>), !cir.ptr<!s32i>
3841
// CHECK: %[[ELE_PTR:.*]] = cir.ptr_stride(%[[ARR_PTR]] : !cir.ptr<!s32i>, %[[IDX]] : !s32i), !cir.ptr<!s32i>
3942
// CHECK: %[[TMP:.*]] = cir.load %[[ELE_PTR]] : !cir.ptr<!s32i>, !s32i
4043
// CHECK" cir.store %[[TMP]], %[[INIT]] : !s32i, !cir.ptr<!s32i>
44+
45+
// CHECK: %[[IDX:.*]] = cir.const #cir.int<1> : !s32i
46+
// CHECK: %[[ARR_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR]] : !cir.ptr<!cir.array<!s32i x 10>>), !cir.ptr<!s32i>
47+
// CHECK: %[[ELE_PTR:.*]] = cir.ptr_stride(%[[ARR_PTR]] : !cir.ptr<!s32i>, %[[IDX]] : !s32i), !cir.ptr<!s32i>
48+
// CHECK: %[[TMP:.*]] = cir.load %[[ELE_PTR]] : !cir.ptr<!s32i>, !s32i
49+
// CHECK" cir.store %[[TMP]], %[[INIT_2]] : !s32i, !cir.ptr<!s32i>
4150
}
4251

4352
void func2() {

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,15 +31,21 @@ int f[5] = {1, 2};
3131

3232
void func() {
3333
int arr[10];
34-
int e = arr[1];
34+
int e = arr[0];
35+
int e2 = arr[1];
3536
}
3637
// CHECK: define void @func()
3738
// CHECK-NEXT: %[[ARR_ALLOCA:.*]] = alloca [10 x i32], i64 1, align 16
3839
// CHECK-NEXT: %[[INIT:.*]] = alloca i32, i64 1, align 4
40+
// CHECK-NEXT: %[[INIT_2:.*]] = alloca i32, i64 1, align 4
3941
// CHECK-NEXT: %[[ARR_PTR:.*]] = getelementptr i32, ptr %[[ARR_ALLOCA]], i32 0
40-
// CHECK-NEXT: %[[ELE_PTR:.*]] = getelementptr i32, ptr %[[ARR_PTR]], i64 1
42+
// CHECK-NEXT: %[[ELE_PTR:.*]] = getelementptr i32, ptr %[[ARR_PTR]], i64 0
4143
// CHECK-NEXT: %[[TMP:.*]] = load i32, ptr %[[ELE_PTR]], align 4
4244
// CHECK-NEXT: store i32 %[[TMP]], ptr %[[INIT]], align 4
45+
// CHECK-NEXT: %[[ARR_PTR:.*]] = getelementptr i32, ptr %[[ARR_ALLOCA]], i32 0
46+
// CHECK-NEXT: %[[ELE_PTR:.*]] = getelementptr i32, ptr %[[ARR_PTR]], i64 1
47+
// CHECK-NEXT: %[[TMP:.*]] = load i32, ptr %[[ELE_PTR]], align 4
48+
// CHECK-NEXT: store i32 %[[TMP]], ptr %[[INIT_2]], align 4
4349

4450
void func2() {
4551
int arr[2] = {5};

0 commit comments

Comments
 (0)