[CIR] Upstream ArraySubscriptExpr for fixed size array #134536
Conversation
AmrDeveloper
requested review from
AaronBallman,
xlauko,
erichkeane,
dkolsen-pgi and
andykaylor
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clangir Author: Amr Hesham (AmrDeveloper) ChangesThis change adds ArraySubscriptExpr for fixed size ArrayType Issue #130197 Full diff: https://github.com/llvm/llvm-project/pull/134536.diff 10 Files Affected:
diff --git a/clang/include/clang/CIR/MissingFeatures.h b/clang/include/clang/CIR/MissingFeatures.h
index 86fdaf1ddaf51..6872f50565721 100644
--- a/clang/include/clang/CIR/MissingFeatures.h
+++ b/clang/include/clang/CIR/MissingFeatures.h
@@ -108,6 +108,7 @@ struct MissingFeatures {
static bool cgFPOptionsRAII() { return false; }
static bool metaDataNode() { return false; }
static bool fastMathFlags() { return false; }
+ static bool emitCheckedInBoundsGEP() { return false; }
// Missing types
static bool dataMemberType() { return false; }
diff --git a/clang/lib/CIR/CodeGen/CIRGenBuilder.cpp b/clang/lib/CIR/CodeGen/CIRGenBuilder.cpp
new file mode 100644
index 0000000000000..2f1940e656172
--- /dev/null
+++ b/clang/lib/CIR/CodeGen/CIRGenBuilder.cpp
@@ -0,0 +1,40 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "CIRGenBuilder.h"
+
+using namespace clang::CIRGen;
+
+mlir::Value CIRGenBuilderTy::maybeBuildArrayDecay(mlir::Location loc,
+ mlir::Value arrayPtr,
+ mlir::Type eltTy) {
+ const auto arrayPtrTy = mlir::cast<cir::PointerType>(arrayPtr.getType());
+ const auto arrayTy = mlir::dyn_cast<cir::ArrayType>(arrayPtrTy.getPointee());
+
+ if (arrayTy) {
+ const cir::PointerType flatPtrTy = getPointerTo(arrayTy.getEltType());
+ return create<cir::CastOp>(loc, flatPtrTy, cir::CastKind::array_to_ptrdecay,
+ arrayPtr);
+ }
+
+ assert(arrayPtrTy.getPointee() == eltTy &&
+ "flat pointee type must match original array element type");
+ return arrayPtr;
+}
+
+mlir::Value CIRGenBuilderTy::getArrayElement(mlir::Location arrayLocBegin,
+ mlir::Location arrayLocEnd,
+ mlir::Value arrayPtr,
+ mlir::Type eltTy, mlir::Value idx,
+ bool shouldDecay) {
+ mlir::Value basePtr = arrayPtr;
+ if (shouldDecay)
+ basePtr = maybeBuildArrayDecay(arrayLocBegin, arrayPtr, eltTy);
+ const mlir::Type flatPtrTy = basePtr.getType();
+ return create<cir::PtrStrideOp>(arrayLocEnd, flatPtrTy, basePtr, idx);
+}
diff --git a/clang/lib/CIR/CodeGen/CIRGenBuilder.h b/clang/lib/CIR/CodeGen/CIRGenBuilder.h
index 61a747254b3d0..e659a8fbf4eff 100644
--- a/clang/lib/CIR/CodeGen/CIRGenBuilder.h
+++ b/clang/lib/CIR/CodeGen/CIRGenBuilder.h
@@ -198,6 +198,19 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
return create<cir::BinOp>(loc, cir::BinOpKind::Div, lhs, rhs);
}
+
+ /// Create a cir.ptr_stride operation to get access to an array element.
+ /// idx is the index of the element to access, shouldDecay is true if the
+ /// result should decay to a pointer to the element type.
+ mlir::Value getArrayElement(mlir::Location arrayLocBegin,
+ mlir::Location arrayLocEnd, mlir::Value arrayPtr,
+ mlir::Type eltTy, mlir::Value idx,
+ bool shouldDecay);
+
+ /// Returns a decayed pointer to the first element of the array
+ /// pointed to by arrayPtr.
+ mlir::Value maybeBuildArrayDecay(mlir::Location loc, mlir::Value arrayPtr,
+ mlir::Type eltTy);
};
} // namespace clang::CIRGen
diff --git a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
index f01e03a89981d..f1a15e46265b5 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExpr.cpp
@@ -12,6 +12,7 @@
#include "Address.h"
#include "CIRGenFunction.h"
+#include "CIRGenModule.h"
#include "CIRGenValue.h"
#include "mlir/IR/BuiltinAttributes.h"
#include "clang/AST/Attr.h"
@@ -232,6 +233,161 @@ LValue CIRGenFunction::emitUnaryOpLValue(const UnaryOperator *e) {
llvm_unreachable("Unknown unary operator kind!");
}
+/// If the specified expr is a simple decay from an array to pointer,
+/// return the array subexpression.
+/// FIXME: this could be abstracted into a commeon AST helper.
+static const Expr *isSimpleArrayDecayOperand(const Expr *e) {
+ // If this isn't just an array->pointer decay, bail out.
+ const auto *castExpr = dyn_cast<CastExpr>(e);
+ if (!castExpr || castExpr->getCastKind() != CK_ArrayToPointerDecay)
+ return nullptr;
+
+ // If this is a decay from variable width array, bail out.
+ const Expr *subExpr = castExpr->getSubExpr();
+ if (subExpr->getType()->isVariableArrayType())
+ return nullptr;
+
+ return subExpr;
+}
+
+static mlir::IntegerAttr getConstantIndexOrNull(mlir::Value idx) {
+ // TODO(cir): should we consider using MLIRs IndexType instead of IntegerAttr?
+ if (auto constantOp = dyn_cast<cir::ConstantOp>(idx.getDefiningOp()))
+ return mlir::dyn_cast<mlir::IntegerAttr>(constantOp.getValue());
+ return {};
+}
+
+static CharUnits getArrayElementAlign(CharUnits arrayAlign, mlir::Value idx,
+ CharUnits eltSize) {
+ // If we have a constant index, we can use the exact offset of the
+ // element we're accessing.
+ const mlir::IntegerAttr constantIdx = getConstantIndexOrNull(idx);
+ if (constantIdx) {
+ const CharUnits offset = constantIdx.getValue().getZExtValue() * eltSize;
+ return arrayAlign.alignmentAtOffset(offset);
+ }
+ // Otherwise, use the worst-case alignment for any element.
+ return arrayAlign.alignmentOfArrayElement(eltSize);
+}
+
+static QualType getFixedSizeElementType(const ASTContext &astContext,
+ const VariableArrayType *vla) {
+ QualType eltType;
+ do {
+ eltType = vla->getElementType();
+ } while ((vla = astContext.getAsVariableArrayType(eltType)));
+ return eltType;
+}
+
+static mlir::Value
+emitArraySubscriptPtr(CIRGenFunction &cgf, mlir::Location beginLoc,
+ mlir::Location endLoc, mlir::Value ptr, mlir::Type eltTy,
+ ArrayRef<mlir::Value> indices, bool inbounds,
+ bool signedIndices, bool shouldDecay,
+ const llvm::Twine &name = "arrayidx") {
+ if (indices.size() > 1) {
+ cgf.cgm.errorNYI("emitArraySubscriptPtr: handle multiple indices");
+ return {};
+ }
+
+ const mlir::Value idx = indices.back();
+ CIRGenModule &cgm = cgf.getCIRGenModule();
+ // TODO(cir): LLVM codegen emits in bound gep check here, is there anything
+ // that would enhance tracking this later in CIR?
+ if (inbounds)
+ assert(!cir::MissingFeatures::emitCheckedInBoundsGEP() && "NYI");
+ return cgm.getBuilder().getArrayElement(beginLoc, endLoc, ptr, eltTy, idx,
+ shouldDecay);
+}
+
+static Address emitArraySubscriptPtr(
+ CIRGenFunction &cgf, mlir::Location beginLoc, mlir::Location endLoc,
+ Address addr, ArrayRef<mlir::Value> indices, QualType eltType,
+ bool inbounds, bool signedIndices, mlir::Location loc, bool shouldDecay,
+ QualType *arrayType = nullptr, const Expr *base = nullptr,
+ const llvm::Twine &name = "arrayidx") {
+
+ // Determine the element size of the statically-sized base. This is
+ // the thing that the indices are expressed in terms of.
+ if (const VariableArrayType *vla =
+ cgf.getContext().getAsVariableArrayType(eltType)) {
+ eltType = getFixedSizeElementType(cgf.getContext(), vla);
+ }
+
+ // We can use that to compute the best alignment of the element.
+ const CharUnits eltSize = cgf.getContext().getTypeSizeInChars(eltType);
+ const CharUnits eltAlign =
+ getArrayElementAlign(addr.getAlignment(), indices.back(), eltSize);
+
+ mlir::Value eltPtr;
+ const mlir::IntegerAttr lastIndex = getConstantIndexOrNull(indices.back());
+ if (!lastIndex) {
+ eltPtr = emitArraySubscriptPtr(cgf, beginLoc, endLoc, addr.getPointer(),
+ addr.getElementType(), indices, inbounds,
+ signedIndices, shouldDecay, name);
+ }
+ const mlir::Type elementType = cgf.convertTypeForMem(eltType);
+ return Address(eltPtr, elementType, eltAlign);
+}
+
+LValue
+CIRGenFunction::emitArraySubscriptExpr(const clang::ArraySubscriptExpr *e) {
+ if (e->getBase()->getType()->isVectorType() &&
+ !isa<ExtVectorElementExpr>(e->getBase())) {
+ cgm.errorNYI(e->getSourceRange(), "emitArraySubscriptExpr: VectorType");
+ return {};
+ }
+
+ if (isa<ExtVectorElementExpr>(e->getBase())) {
+ cgm.errorNYI(e->getSourceRange(),
+ "emitArraySubscriptExpr: ExtVectorElementExpr");
+ return {};
+ }
+
+ if (getContext().getAsVariableArrayType(e->getType())) {
+ cgm.errorNYI(e->getSourceRange(),
+ "emitArraySubscriptExpr: VariableArrayType");
+ return {};
+ }
+
+ if (e->getType()->getAs<ObjCObjectType>()) {
+ cgm.errorNYI(e->getSourceRange(), "emitArraySubscriptExpr: ObjCObjectType");
+ return {};
+ }
+
+ // The index must always be an integer, which is not an aggregate. Emit it
+ // in lexical order (this complexity is, sadly, required by C++17).
+ assert((e->getIdx() == e->getLHS() || e->getIdx() == e->getRHS()) &&
+ "index was neither LHS nor RHS");
+ const mlir::Value idx = emitScalarExpr(e->getIdx());
+ const QualType idxTy = e->getIdx()->getType();
+ const bool signedIndices = idxTy->isSignedIntegerOrEnumerationType();
+
+ if (const Expr *array = isSimpleArrayDecayOperand(e->getBase())) {
+ LValue arrayLV;
+ if (const auto *ase = dyn_cast<ArraySubscriptExpr>(array))
+ arrayLV = emitArraySubscriptExpr(ase);
+ else
+ arrayLV = emitLValue(array);
+
+ // Propagate the alignment from the array itself to the result.
+ QualType arrayType = array->getType();
+ const Address addr = emitArraySubscriptPtr(
+ *this, cgm.getLoc(array->getBeginLoc()), cgm.getLoc(array->getEndLoc()),
+ arrayLV.getAddress(), {idx}, e->getType(),
+ !getLangOpts().isSignedOverflowDefined(), signedIndices,
+ cgm.getLoc(e->getExprLoc()), /*shouldDecay=*/true, &arrayType,
+ e->getBase());
+
+ return LValue::makeAddr(addr, e->getType());
+ }
+
+ // The base must be a pointer; emit it with an estimate of its alignment.
+ cgm.errorNYI(e->getSourceRange(),
+ "emitArraySubscriptExpr: The base must be a pointer");
+ return {};
+}
+
LValue CIRGenFunction::emitBinaryOperatorLValue(const BinaryOperator *e) {
// Comma expressions just emit their LHS then their RHS as an l-value.
if (e->getOpcode() == BO_Comma) {
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
index 3863d21487531..09794a782b0be 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
@@ -157,6 +157,15 @@ class ScalarExprEmitter : public StmtVisitor<ScalarExprEmitter, mlir::Value> {
mlir::Value VisitCastExpr(CastExpr *e);
+ mlir::Value VisitArraySubscriptExpr(ArraySubscriptExpr *e) {
+ if (e->getBase()->getType()->isVectorType()) {
+ assert(!cir::MissingFeatures::scalableVectors() &&
+ "NYI: index into scalable vector");
+ }
+ // Just load the lvalue formed by the subscript expression.
+ return emitLoadOfLValue(e);
+ }
+
mlir::Value VisitExplicitCastExpr(ExplicitCastExpr *e) {
return VisitCastExpr(e);
}
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
index 2465ccffd19d6..b518a3286f7aa 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
@@ -444,6 +444,8 @@ LValue CIRGenFunction::emitLValue(const Expr *e) {
std::string("l-value not implemented for '") +
e->getStmtClassName() + "'");
return LValue();
+ case Expr::ArraySubscriptExprClass:
+ return emitArraySubscriptExpr(cast<ArraySubscriptExpr>(e));
case Expr::UnaryOperatorClass:
return emitUnaryOpLValue(cast<UnaryOperator>(e));
case Expr::BinaryOperatorClass:
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index 1bedbe28ae625..f279a821b3292 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -387,6 +387,8 @@ class CIRGenFunction : public CIRGenTypeCache {
/// should be returned.
RValue emitAnyExpr(const clang::Expr *e);
+ LValue emitArraySubscriptExpr(const clang::ArraySubscriptExpr *e);
+
AutoVarEmission emitAutoVarAlloca(const clang::VarDecl &d);
/// Emit code and set up symbol table for a variable declaration with auto,
diff --git a/clang/lib/CIR/CodeGen/CMakeLists.txt b/clang/lib/CIR/CodeGen/CMakeLists.txt
index da8d63ca569af..90544582e4415 100644
--- a/clang/lib/CIR/CodeGen/CMakeLists.txt
+++ b/clang/lib/CIR/CodeGen/CMakeLists.txt
@@ -8,6 +8,7 @@ get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
add_clang_library(clangCIR
CIRGenerator.cpp
+ CIRGenBuilder.cpp
CIRGenDecl.cpp
CIRGenExpr.cpp
CIRGenExprAggregate.cpp
diff --git a/clang/test/CIR/CodeGen/array.cpp b/clang/test/CIR/CodeGen/array.cpp
index 0d28ebc66f83c..387c32742b891 100644
--- a/clang/test/CIR/CodeGen/array.cpp
+++ b/clang/test/CIR/CodeGen/array.cpp
@@ -29,8 +29,15 @@ int f[5] = {1, 2};
void func() {
int arr[10];
-
// CHECK: %[[ARR:.*]] = cir.alloca !cir.array<!s32i x 10>, !cir.ptr<!cir.array<!s32i x 10>>, ["arr"]
+
+ int e = arr[1];
+ // CHECK: %[[INIT:.*]] = cir.alloca !s32i, !cir.ptr<!s32i>, ["e", init]
+ // CHECK: %[[IDX:.*]] = cir.const #cir.int<1> : !s32i
+ // CHECK: %[[ARR_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR]] : !cir.ptr<!cir.array<!s32i x 10>>), !cir.ptr<!s32i>
+ // CHECK: %[[ELE_PTR:.*]] = cir.ptr_stride(%[[ARR_PTR]] : !cir.ptr<!s32i>, %[[IDX]] : !s32i), !cir.ptr<!s32i>
+ // CHECK: %[[TMP:.*]] = cir.load %[[ELE_PTR]] : !cir.ptr<!s32i>, !s32i
+ // CHECK" cir.store %[[TMP]], %[[INIT]] : !s32i, !cir.ptr<!s32i>
}
void func2() {
@@ -69,6 +76,7 @@ void func4() {
int arr[2][1] = {{5}, {6}};
// CHECK: %[[ARR:.*]] = cir.alloca !cir.array<!cir.array<!s32i x 1> x 2>, !cir.ptr<!cir.array<!cir.array<!s32i x 1> x 2>>, ["arr", init]
+ // CHECK: %[[INIT:.*]] = cir.alloca !s32i, !cir.ptr<!s32i>, ["e", init]
// CHECK: %[[ARR_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR]] : !cir.ptr<!cir.array<!cir.array<!s32i x 1> x 2>>), !cir.ptr<!cir.array<!s32i x 1>>
// CHECK: %[[ARR_0_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR_PTR]] : !cir.ptr<!cir.array<!s32i x 1>>), !cir.ptr<!s32i>
// CHECK: %[[V_0_0:.*]] = cir.const #cir.int<5> : !s32i
@@ -78,6 +86,17 @@ void func4() {
// CHECK: %[[ARR_1_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR_1]] : !cir.ptr<!cir.array<!s32i x 1>>), !cir.ptr<!s32i>
// CHECK: %[[V_1_0:.*]] = cir.const #cir.int<6> : !s32i
// CHECK: cir.store %[[V_1_0]], %[[ARR_1_PTR]] : !s32i, !cir.ptr<!s32i>
+
+ int e = arr[1][0];
+
+ // CHECK: %[[IDX:.*]] = cir.const #cir.int<0> : !s32i
+ // CHECK: %[[IDX_1:.*]] = cir.const #cir.int<1> : !s32i
+ // CHECK: %[[ARR_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR]] : !cir.ptr<!cir.array<!cir.array<!s32i x 1> x 2>>), !cir.ptr<!cir.array<!s32i x 1>>
+ // CHECK: %[[ARR_1:.*]] = cir.ptr_stride(%[[ARR_PTR]] : !cir.ptr<!cir.array<!s32i x 1>>, %[[IDX_1]] : !s32i), !cir.ptr<!cir.array<!s32i x 1>>
+ // CHECK: %[[ARR_1_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR_1]] : !cir.ptr<!cir.array<!s32i x 1>>), !cir.ptr<!s32i>
+ // CHECK: %[[ELE_0:.*]] = cir.ptr_stride(%[[ARR_1_PTR]] : !cir.ptr<!s32i>, %[[IDX]] : !s32i), !cir.ptr<!s32i>
+ // CHECK: %[[TMP:.*]] = cir.load %[[ELE_0]] : !cir.ptr<!s32i>, !s32i
+ // CHECK: cir.store %[[TMP]], %[[INIT]] : !s32i, !cir.ptr<!s32i>
}
void func5() {
diff --git a/clang/test/CIR/Lowering/array.cpp b/clang/test/CIR/Lowering/array.cpp
index e1c977eb43141..8519525db07ec 100644
--- a/clang/test/CIR/Lowering/array.cpp
+++ b/clang/test/CIR/Lowering/array.cpp
@@ -31,12 +31,18 @@ int f[5] = {1, 2};
void func() {
int arr[10];
+ int e = arr[1];
}
// CHECK: define void @func()
-// CHECK-NEXT: alloca [10 x i32], i64 1, align 16
+// CHECK-NEXT: %[[ARR_ALLOCA:.*]] = alloca [10 x i32], i64 1, align 16
+// CHECK-NEXT: %[[INIT:.*]] = alloca i32, i64 1, align 4
+// CHECK-NEXT: %[[ARR_PTR:.*]] = getelementptr i32, ptr %[[ARR_ALLOCA]], i32 0
+// CHECK-NEXT: %[[ELE_PTR:.*]] = getelementptr i32, ptr %[[ARR_PTR]], i64 1
+// CHECK-NEXT: %[[TMP:.*]] = load i32, ptr %[[ELE_PTR]], align 4
+// CHECK-NEXT: store i32 %[[TMP]], ptr %[[INIT]], align 4
void func2() {
- int arr2[2] = {5};
+ int arr[2] = {5};
}
// CHECK: define void @func2()
// CHECK: %[[ARR_ALLOCA:.*]] = alloca [2 x i32], i64 1, align 4
@@ -61,19 +67,27 @@ void func3() {
// CHECK: store i32 6, ptr %[[ELE_1_PTR]], align 4
void func4() {
- int arr4[2][1] = {{5}, {6}};
+ int arr[2][1] = {{5}, {6}};
+ int e = arr[1][0];
}
// CHECK: define void @func4()
// CHECK: %[[ARR_ALLOCA:.*]] = alloca [2 x [1 x i32]], i64 1, align 4
-// CHECK: %[[ARR_0:.*]] = getelementptr [1 x i32], ptr %[[ARR_ALLOCA]], i32 0
-// CHECK: %[[ARR_0_ELE_0:.*]] = getelementptr i32, ptr %[[ARR_0]], i32 0
-// CHECK: store i32 5, ptr %[[ARR_0_ELE_0]], align 4
-// CHECK: %[[ARR_1:.*]] = getelementptr [1 x i32], ptr %2, i64 1
-// CHECK: %[[ARR_0_ELE_0:.*]] = getelementptr i32, ptr %[[ARR_1]], i32 0
-// CHECK: store i32 6, ptr %[[ARR_0_ELE_0]], align 4
+// CHECK: %[[INIT:.*]] = alloca i32, i64 1, align 4
+// CHECK: %[[ARR_PTR:.*]] = getelementptr [1 x i32], ptr %[[ARR_ALLOCA]], i32 0
+// CHECK: %[[ARR_0_0:.*]] = getelementptr i32, ptr %[[ARR_PTR]], i32 0
+// CHECK: store i32 5, ptr %[[ARR_0_0]], align 4
+// CHECK: %[[ARR_1:.*]] = getelementptr [1 x i32], ptr %[[ARR_PTR]], i64 1
+// CHECK: %[[ARR_1_0:.*]] = getelementptr i32, ptr %[[ARR_1]], i32 0
+// CHECK: store i32 6, ptr %[[ARR_1_0]], align 4
+// CHECK: %[[ARR_PTR:.*]] = getelementptr [1 x i32], ptr %[[ARR_ALLOCA]], i32 0
+// CHECK: %[[ARR_1:.*]] = getelementptr [1 x i32], ptr %[[ARR_PTR]], i64 1
+// CHECK: %[[ARR_1_0:.*]] = getelementptr i32, ptr %[[ARR_1]], i32 0
+// CHECK: %[[ELE_PTR:.*]] = getelementptr i32, ptr %[[ARR_1_0]], i64 0
+// CHECK: %[[TMP:.*]] = load i32, ptr %[[ELE_PTR]], align 4
+// CHECK: store i32 %[[TMP]], ptr %[[INIT]], align 4
void func5() {
- int arr5[2][1] = {{5}};
+ int arr[2][1] = {{5}};
}
// CHECK: define void @func5()
// CHECK: %[[ARR_ALLOCA:.*]] = alloca [2 x [1 x i32]], i64 1, align 4
|
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
| @@ -232,6 +233,161 @@ LValue CIRGenFunction::emitUnaryOpLValue(const UnaryOperator *e) { | |||
| llvm_unreachable("Unknown unary operator kind!"); | |||
| } | |||
|
|
|||
| /// If the specified expr is a simple decay from an array to pointer, | |||
| /// return the array subexpression. | |||
| /// FIXME: this could be abstracted into a commeon AST helper. | |||
There was a problem hiding this comment.
| /// FIXME: this could be abstracted into a commeon AST helper. | |
| /// FIXME: this could be abstracted into a common AST helper. |
|
|
||
| // If this is a decay from variable width array, bail out. | ||
| const Expr *subExpr = castExpr->getSubExpr(); | ||
| if (subExpr->getType()->isVariableArrayType()) |
There was a problem hiding this comment.
Does this need an Assert/NYI/etc here? Seems like the purpose of MissingFeatures
There was a problem hiding this comment.
As far as I understand, it's internal to be nullptr, not a missing feature or implementation similar to Clang
llvm-project/clang/lib/CodeGen/CGExpr.cpp
Lines 4054 to 4068 in 783201b
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
| static mlir::IntegerAttr getConstantIndexOrNull(mlir::Value idx) { | ||
| // TODO(cir): should we consider using MLIRs IndexType instead of IntegerAttr? | ||
| if (auto constantOp = dyn_cast<cir::ConstantOp>(idx.getDefiningOp())) | ||
| return mlir::dyn_cast<mlir::IntegerAttr>(constantOp.getValue()); |
There was a problem hiding this comment.
Does this ever return non-null? I would expect the constantOp to have a cir::IntAttr rather than an mlir::IntegerAttr. I'm not sure the former can be cast to the latter.
| const mlir::IntegerAttr constantIdx = getConstantIndexOrNull(idx); | ||
| if (constantIdx) { | ||
| const CharUnits offset = constantIdx.getValue().getZExtValue() * eltSize; | ||
| return arrayAlign.alignmentAtOffset(offset); |
There was a problem hiding this comment.
Do we have a test case where this would return something different than alignmentOfArrayElement(eltSize)?
There was a problem hiding this comment.
I am thinking for test case that can be used
There was a problem hiding this comment.
In order to find a test case for this, I modified the equivalent classic clang codegen implementation so that it would always go to the else case here. That caused 15 tests to fail, but we don't have enough support implemented to run any of the failing tests with CIR. There were 12 OpenMP failures, two PowerPC intrinsics, and one HLSL.
clang/test/CIR/CodeGen/array.cpp
Outdated
| // CHECK: %[[ARR_PTR:.*]] = cir.cast(array_to_ptrdecay, %[[ARR]] : !cir.ptr<!cir.array<!s32i x 10>>), !cir.ptr<!s32i> | ||
| // CHECK: %[[ELE_PTR:.*]] = cir.ptr_stride(%[[ARR_PTR]] : !cir.ptr<!s32i>, %[[IDX]] : !s32i), !cir.ptr<!s32i> | ||
| // CHECK: %[[TMP:.*]] = cir.load %[[ELE_PTR]] : !cir.ptr<!s32i>, !s32i | ||
| // CHECK" cir.store %[[TMP]], %[[INIT]] : !s32i, !cir.ptr<!s32i> |
There was a problem hiding this comment.
Can you also add RUN lines and checks for emitting LLVM IR and emitting LLVM IR without using CIR?
There was a problem hiding this comment.
In this case, do we still need lowering/array.cpp? can we use this file for the 3 tests together and remove the other one
There was a problem hiding this comment.
I think eventually we could get rid of all the lowering tests, but we'd need to make sure what they are testing is fully covered in the codegen tests.
@bcardosolopes @lanza Does this seem reasonable to you? Would it create problems for rebasing the incubator?
There was a problem hiding this comment.
I think eventually we could get rid of all the lowering tests
Even though we don't want encourage people to write .cir lowering tests (because they are hard to update, etc) and c/c++ has better end-to-end coverage, we probably still need a place for tests with .cir suffix whenever they make sense (handy for testing against niche bugs, regressions and whatever corner case make sense). I'm not particular with the "Lowering" name, but I think it's probably nice if those tests are not directly under CodeGen. Related to this PR: I also don't any purpose on keeping c/c++ source files under Lowering.
Would it create problems for rebasing the incubator?
I don't think so, @lanza can you confirm?
There was a problem hiding this comment.
The incubator is completely disjoint from upstream. There's no way to reconcile the two versions of the project via a rebase given how different ClangIR was to start. CIRGen was entirely different (it was more of an AST walk than a clone of CodeGen), the CIR dialect was in MLIR, CIR was first implemented as a conditional functionality on top of the CFG and not as a codegen-like path, etc. So feel free to do whatever you'd like. If you need the two paths to agree at some point it would have to be via manual intervention adding new patches on top of the incubator.
For context, the rebsaing process is (in psuedo python)
git checkout $(git merge-base incubator/main upstream/main)
for commit in reverse(find_all_upstream_commits):
git revert commit
git cherry-pick all_of_incubator -x 'ninja -C build check-clang-cir'
So feel free to introduce whatever changes you want to upstream, but you won't get them in the incubator and so you'll need to apply them manually if necessary.
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
| mlir::Location beginLoc, | ||
| mlir::Location endLoc, mlir::Value ptr, | ||
| mlir::Type eltTy, mlir::Value idx, | ||
| bool inbounds, bool signedIndices, |
There was a problem hiding this comment.
The signedIndices parameter isn't needed here.
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
|
|
||
| mlir::Value eltPtr; | ||
| const mlir::IntegerAttr index = getConstantIndexOrNull(idx); | ||
| if (!index) { |
There was a problem hiding this comment.
The incubator is also checking (!CGF.IsInPreservedAIRegion && !isPreserveAIArrayBase(CGF, Base))) here. Can you add a MissingFeatures assert for that?
There was a problem hiding this comment.
Not sure if I can easily add MissingFeatures without conflict with the else branch, maybe I should upstream the basic implementation of this failed and function with errorNYI in their cases
There was a problem hiding this comment.
The MissingFeatures assert is effectively a noop. You're asserting something which is statically true. It just serves as a placeholder in the code to help us find places where changes need to be made in the future.
| if (!index) { | |
| if (!index) { | |
| assert(!cir::MissingFeatures::preservedAccessIndexRegion()); |
There was a problem hiding this comment.
Thats right, thank you @andykaylor
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
| emitArraySubscriptPtr(CIRGenFunction &cgf, mlir::Location beginLoc, | ||
| mlir::Location endLoc, Address addr, QualType eltType, | ||
| mlir::Value idx, bool inbounds, mlir::Location loc, | ||
| bool shouldDecay, QualType *arrayType, const Expr *base) { |
There was a problem hiding this comment.
The arrayType and base parameters aren't referenced. They can be removed until they are needed.
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
| static mlir::IntegerAttr getConstantIndexOrNull(mlir::Value idx) { | ||
| // TODO(cir): should we consider using MLIRs IndexType instead of IntegerAttr? | ||
| if (auto constantOp = dyn_cast<cir::ConstantOp>(idx.getDefiningOp())) | ||
| return mlir::dyn_cast<mlir::IntegerAttr>(constantOp.getValue()); |
There was a problem hiding this comment.
| return mlir::dyn_cast<mlir::IntegerAttr>(constantOp.getValue()); | |
| return mlir::dyn_cast<cir::IntAttr>(constantOp.getValue()); |
I just tested this in the incubator, and as I suspected, this function was always returning null because our constants don't use mlir::IntegerAttr.
There was a problem hiding this comment.
I updated the function signature and cast, I will understand this part more in the incubator because that means it was only depend on that condition (!CGF.IsInPreservedAIRegion && !isPreserveAIArrayBase(CGF, Base)) not the index itself
There was a problem hiding this comment.
I am thinking to upstream at least isPreserveAIArrayBase because now even this NYI message is wrong, it should be related to PreserveAIArray so I think it better to upstream isPreserveAIArrayBase so I can make the else branch related to itcgf.cgm.errorNYI("emitArraySubscriptExpr: Non Constant Index");
And in that case will add test to cover array[x]
What do you think? @andykaylor
There was a problem hiding this comment.
If I understand correctly, cgf.isInPreservedAIRegion will never be true until we support the associated builtin function (__builtin_preserve_access_index), which isn't supported yet in the incubator and isn't a high priority and might not be relevant to CIR at all. I don't see any reason to upstream any of that yet.
But you're right that the check in emitArraySubscriptPtr is wrong as it currently appears in this PR. It appears that there is no reason to be checking for a constant index as we are there and we should be unconditionally calling the code in the true case. If you can support a variable index with no other changes, go ahead. Otherwise, leave it for a subsequent patch.
There was a problem hiding this comment.
@andykaylor That makes sense, I updated the test to include an array with a variable index
clang/lib/CIR/CodeGen/CIRGenExpr.cpp
Outdated
| return subExpr; | ||
| } | ||
|
|
||
| static mlir::IntegerAttr getConstantIndexOrNull(mlir::Value idx) { |
There was a problem hiding this comment.
| static mlir::IntegerAttr getConstantIndexOrNull(mlir::Value idx) { | |
| static cir::IntAttr getConstantIndexOrNull(mlir::Value idx) { |
| const mlir::IntegerAttr constantIdx = getConstantIndexOrNull(idx); | ||
| if (constantIdx) { | ||
| const CharUnits offset = constantIdx.getValue().getZExtValue() * eltSize; | ||
| return arrayAlign.alignmentAtOffset(offset); |
There was a problem hiding this comment.
In order to find a test case for this, I modified the equivalent classic clang codegen implementation so that it would always go to the else case here. That caused 15 tests to fail, but we don't have enough support implemented to run any of the failing tests with CIR. There were 12 OpenMP failures, two PowerPC intrinsics, and one HLSL.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
AmrDeveloper
force-pushed
the
cir_upstream_array_index
branch
from
c2f5f0f to
fb439ca
Compare
| // LLVM-NEXT: %[[ELE_PTR:.*]] = getelementptr i32, ptr %[[ARR_PTR]], i64 1 | ||
| // LLVM-NEXT: %[[TMP:.*]] = load i32, ptr %[[ELE_PTR]], align 4 | ||
|
|
||
| // OGCG: %arr = alloca [10 x i32], align 16 |
There was a problem hiding this comment.
The variable names should be pattern-matched here as they are in the LLVM checks, but that can be done in a follow-up patch.
There was a problem hiding this comment.
Right, i will create a follow up patch for it after mering this
Thanks
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/16681 Here is the relevant piece of the build log for the reference |
This change adds ArraySubscriptExpr for fixed size ArrayType Issue llvm#130197
Follow-up patch to improve variable names in LLVM and OGCG in #134536
…135427) Follow-up patch to improve variable names in LLVM and OGCG in llvm/llvm-project#134536
This change adds ArraySubscriptExpr for fixed size ArrayType Issue llvm#130197
Follow-up patch to improve variable names in LLVM and OGCG in llvm#134536
This change adds ArraySubscriptExpr for fixed size ArrayType
Issue #130197