[CIR] Upstream basic support for ArrayType #130502

AmrDeveloper · 2025-03-09T18:28:02Z

This change adds the basic support for ArrayType

AmrDeveloper · 2025-03-09T18:28:09Z

There will be two upcoming patches.

1 - Initializing const array and zero initializing support (Depends on upstreaming ConstantEmitter and ZeroAttr)
2 - Dynamic-size arrays, and accessing arrays using the cir.ptr_stride operation (Depends on upstreaming CastOp)

llvmbot · 2025-03-09T18:28:34Z

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangir

Author: Amr Hesham (AmrDeveloper)

Changes

This change adds the basic support for ArrayType

Issue #130197

Full diff: https://github.com/llvm/llvm-project/pull/130502.diff

7 Files Affected:

(modified) clang/include/clang/CIR/Dialect/IR/CIRAttrs.h (+4)
(modified) clang/include/clang/CIR/Dialect/IR/CIRTypes.td (+20-1)
(modified) clang/lib/CIR/CodeGen/CIRGenBuilder.h (+8)
(modified) clang/lib/CIR/CodeGen/CIRGenTypes.cpp (+12)
(modified) clang/lib/CIR/Dialect/IR/CIRDialect.cpp (+3)
(modified) clang/lib/CIR/Dialect/IR/CIRTypes.cpp (+16)
(added) clang/test/CIR/CodeGen/array.cpp (+12)

diff --git a/clang/include/clang/CIR/Dialect/IR/CIRAttrs.h b/clang/include/clang/CIR/Dialect/IR/CIRAttrs.h
index 438fb7d09608d..1451ea47c50c8 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRAttrs.h
+++ b/clang/include/clang/CIR/Dialect/IR/CIRAttrs.h
@@ -30,6 +30,10 @@ class VarDecl;
 class RecordDecl;
 } // namespace clang
 
+namespace cir {
+class ArrayType;
+} // namespace cir
+
 #define GET_ATTRDEF_CLASSES
 #include "clang/CIR/Dialect/IR/CIROpsAttributes.h.inc"
 
diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
index a78e5eae08e33..b2bac643b62da 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td
@@ -280,6 +280,25 @@ def CIR_BoolType :
   }];
 }
 
+//===----------------------------------------------------------------------===//
+// ArrayType
+//===----------------------------------------------------------------------===//
+
+def CIR_ArrayType : CIR_Type<"Array", "array",
+    [DeclareTypeInterfaceMethods<DataLayoutTypeInterface>]> {
+
+  let summary = "CIR array type";
+  let description = [{
+    `CIR.array` represents C/C++ constant arrays.
+  }];
+
+  let parameters = (ins "mlir::Type":$eltType, "uint64_t":$size);
+
+  let assemblyFormat = [{
+    `<` $eltType `x` $size `>`
+  }];
+}
+
 //===----------------------------------------------------------------------===//
 // FuncType
 //===----------------------------------------------------------------------===//
@@ -386,7 +405,7 @@ def VoidPtr : Type<
 //===----------------------------------------------------------------------===//
 
 def CIR_AnyType : AnyTypeOf<[
-  CIR_VoidType, CIR_BoolType, CIR_IntType, CIR_AnyFloat, CIR_PointerType,
+  CIR_VoidType, CIR_BoolType, CIR_ArrayType, CIR_IntType, CIR_AnyFloat, CIR_PointerType,
   CIR_FuncType
 ]>;
 
diff --git a/clang/lib/CIR/CodeGen/CIRGenBuilder.h b/clang/lib/CIR/CodeGen/CIRGenBuilder.h
index 01d56963883cc..250192ec254d1 100644
--- a/clang/lib/CIR/CodeGen/CIRGenBuilder.h
+++ b/clang/lib/CIR/CodeGen/CIRGenBuilder.h
@@ -33,6 +33,14 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
       llvm_unreachable("NYI: PPC double-double format for long double");
     llvm_unreachable("Unsupported format for long double");
   }
+
+  bool isSized(mlir::Type ty) {
+    if (mlir::isa<cir::PointerType, cir::ArrayType, cir::BoolType,
+                  cir::IntType>(ty))
+      return true;
+    assert(0 && "Unimplemented size for type");
+    return false;
+  }
 };
 
 } // namespace clang::CIRGen
diff --git a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
index dcfaaedc2ef57..78e894dee0071 100644
--- a/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenTypes.cpp
@@ -202,6 +202,18 @@ mlir::Type CIRGenTypes::convertType(QualType type) {
     break;
   }
 
+  case Type::ConstantArray: {
+    const ConstantArrayType *arrTy = cast<ConstantArrayType>(ty);
+    mlir::Type elemTy = convertTypeForMem(arrTy->getElementType());
+
+    // FIXME: In LLVM, "lower arrays of undefined struct type to arrays of
+    // i8 just to have a concrete type". Not sure this makes sense in CIR yet.
+    assert(builder.isSized(elemTy) && "not implemented");
+    resultType = cir::ArrayType::get(builder.getContext(), elemTy,
+                                     arrTy->getSize().getZExtValue());
+    break;
+  }
+
   case Type::FunctionNoProto:
   case Type::FunctionProto:
     resultType = convertFunctionTypeInternal(type);
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 5ad369b40cda1..1e078e2797270 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -166,6 +166,9 @@ static LogicalResult checkConstantTypes(mlir::Operation *op, mlir::Type opType,
     return success();
   }
 
+  if (mlir::isa<cir::ArrayType>(opType))
+    return success();
+
   assert(isa<TypedAttr>(attrType) && "What else could we be looking at here?");
   return op->emitOpError("global with type ")
          << cast<TypedAttr>(attrType).getType() << " not yet supported";
diff --git a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp
index 8bdde54ad41f6..6291297492227 100644
--- a/clang/lib/CIR/Dialect/IR/CIRTypes.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRTypes.cpp
@@ -369,6 +369,22 @@ BoolType::getABIAlignment(const ::mlir::DataLayout &dataLayout,
   return 1;
 }
 
+//===----------------------------------------------------------------------===//
+//  Definitions
+//===----------------------------------------------------------------------===//
+
+llvm::TypeSize
+ArrayType::getTypeSizeInBits(const ::mlir::DataLayout &dataLayout,
+                             ::mlir::DataLayoutEntryListRef params) const {
+  return getSize() * dataLayout.getTypeSizeInBits(getEltType());
+}
+
+uint64_t
+ArrayType::getABIAlignment(const ::mlir::DataLayout &dataLayout,
+                           ::mlir::DataLayoutEntryListRef params) const {
+  return dataLayout.getTypeABIAlignment(getEltType());
+}
+
 //===----------------------------------------------------------------------===//
 // PointerType Definitions
 //===----------------------------------------------------------------------===//
diff --git a/clang/test/CIR/CodeGen/array.cpp b/clang/test/CIR/CodeGen/array.cpp
new file mode 100644
index 0000000000000..3a5950ffe3d87
--- /dev/null
+++ b/clang/test/CIR/CodeGen/array.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o - 2>&1 | FileCheck %s
+
+int a[10];
+// CHECK: cir.global external @a : !cir.array<!cir.int<s, 32> x 10>
+
+extern int b[10];
+// CHECK: cir.global external @b : !cir.array<!cir.int<s, 32> x 10>
+
+void f() {
+  int c[10];
+  // CHECK: %[[ARR:.*]] = cir.alloca !cir.array<!cir.int<s, 32> x 10>, !cir.ptr<!cir.array<!cir.int<s, 32> x 10>>, ["c"]
+}

clang/lib/CIR/CodeGen/CIRGenBuilder.h

erichkeane · 2025-03-10T14:31:05Z

clang/lib/CIR/CodeGen/CIRGenTypes.cpp

+
+    // FIXME: In LLVM, "lower arrays of undefined struct type to arrays of
+    // i8 just to have a concrete type". Not sure this makes sense in CIR yet.
+    assert(builder.isSized(elemTy) && "not implemented");


This should probably use the cgm.errorNYI, then just come up with a way to recover. That said, this is exactly why having isSized assert is awkward here.

Okey, thats make sense and i will try to recover with SInt32Ty

andykaylor

Can you also add a test for lowering to LLVM IR?

andykaylor · 2025-03-10T15:54:32Z

clang/include/clang/CIR/Dialect/IR/CIRTypes.td

@@ -386,7 +405,7 @@ def VoidPtr : Type<
 //===----------------------------------------------------------------------===//

 def CIR_AnyType : AnyTypeOf<[
-  CIR_VoidType, CIR_BoolType, CIR_IntType, CIR_AnyFloat, CIR_PointerType,
+  CIR_VoidType, CIR_BoolType, CIR_ArrayType, CIR_IntType, CIR_AnyFloat, CIR_PointerType,


I think this line is over 80 characters. It should wrap to avoid that.

andykaylor · 2025-03-10T15:58:57Z

clang/test/CIR/CodeGen/array.cpp

@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o - 2>&1 | FileCheck %s


Will multidimensional arrays and array function arguments work at this point? If so, can you add tests for those?

clang/lib/CIR/CodeGen/CIRGenBuilder.h

andykaylor · 2025-03-10T16:06:41Z

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

@@ -166,6 +166,9 @@ static LogicalResult checkConstantTypes(mlir::Operation *op, mlir::Type opType,
    return success();
  }

+  if (mlir::isa<cir::ArrayType>(opType))


I don't see this in the incubator implementation of this function. Why is this here?

andykaylor · 2025-03-10T16:09:11Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+//===----------------------------------------------------------------------===//
+
+llvm::TypeSize
+ArrayType::getTypeSizeInBits(const ::mlir::DataLayout &dataLayout,


Are these functions being called with any of the test cases? I don't see any checks that would be related to this.

I was thinking of sizeof operator but it's not implemented yet, is there is another way to test the size in lit test

My point was that if the code isn't being called, you should leave it out until it is needed.

Yes, but it's part of the DataLayoutTypeInterface interface so we must implement it, I can test it after merging the sizeof PR #130847

I see. Thanks for the clarification.

clang/include/clang/CIR/Dialect/IR/CIRTypes.td

andykaylor · 2025-03-11T21:20:35Z

clang/lib/CIR/CodeGen/CIRGenTypes.cpp

+    mlir::Type elemTy = convertTypeForMem(arrTy->getElementType());
+
+    if (!builder.isSized(elemTy)) {
+      cgm.errorNYI(SourceLocation(), "unimplemented size for type", type);


I don't like the error being handled this way. If you're going to report an error about unimplemented types here, the check for the types should be here. If someone were to change the implementation of isSized(), they would have no way to know that this code needed to be updated.

andykaylor · 2025-03-11T21:30:12Z

clang/lib/CIR/CodeGen/CIRGenBuilder.h

@@ -33,6 +33,11 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
      llvm_unreachable("NYI: PPC double-double format for long double");
    llvm_unreachable("Unsupported format for long double");
  }
+
+  bool isSized(mlir::Type ty) {
+    return mlir::isa<cir::PointerType, cir::ArrayType, cir::BoolType,


I think this was better with the assert for other types. This function should handle all types that can occur in the current form of CIR. As we add new types, this function should be updated to indicate if they are sized or not. So, this isn't quite like other errorNYI cases. An assert here is reasonable as a permanent part of the code.

Yes i will update it, and i think no need for NYI or other assert when calling it because in call cases we will not need to recover

andykaylor · 2025-03-11T21:31:56Z

clang/lib/CIR/Dialect/IR/CIRTypes.cpp

+//===----------------------------------------------------------------------===//
+
+llvm::TypeSize
+ArrayType::getTypeSizeInBits(const ::mlir::DataLayout &dataLayout,


My point was that if the code isn't being called, you should leave it out until it is needed.

andykaylor · 2025-03-11T21:32:28Z

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

@@ -572,6 +572,10 @@ static void prepareTypeConverter(mlir::LLVMTypeConverter &converter,

    return mlir::LLVM::LLVMPointerType::get(type.getContext(), targetAS);
  });
+  converter.addConversion([&](cir::ArrayType type) -> mlir::Type {
+    auto ty = convertTypeForMemory(converter, dataLayout, type.getEltType());


Suggested change

auto ty = convertTypeForMemory(converter, dataLayout, type.getEltType());

mlir::Type ty = convertTypeForMemory(converter, dataLayout, type.getEltType());

andykaylor · 2025-03-11T21:34:19Z

clang/test/CIR/CodeGen/array.cpp

+// CHECK: cir.global external @a : !cir.array<!cir.int<s, 32> x 10>
+
+int aa[10][10];
+// CHECK: cir.global external @aa : !cir.array<!cir.array<!cir.int<s, 32> x 10> x 10>


Use different sizes for the dimensions here so we can verify that the CIR generated has them in the right order.

andykaylor · 2025-03-11T21:35:32Z

clang/test/CIR/CodeGen/array.cpp

+}
+
+void f2(int p[10]) {}
+// CHECK: cir.func @f2(%arg0: !cir.ptr<!cir.int<s, 32>>


Can you also check the cir.alloca generated in this case?

andykaylor

I have just one minor comment on the update. Please wait for @erichkeane to review again, but this looks good to me.

andykaylor · 2025-03-11T22:30:34Z

clang/test/CIR/Lowering/array.cpp

+void f() {
+  int l[10];
+}
+// CHECK: alloca [10 x i32], i64 1, align 16


I think it's a good practice to have a check for the function name to establish that we're seeing the alloca in the correct context.

erichkeane

My concern is still not fixed, and Andy/Bruno still have comments here, so I don't believe this to be ready yet.

clang/lib/CIR/CodeGen/CIRGenBuilder.h

AmrDeveloper · 2025-03-12T20:17:38Z

My concern is still not fixed, and Andy/Bruno still have comments here, so I don't believe this to be ready yet.

I already addressed all comments except the test for Size and it's need sizeof PR to merge, and the assert line which we need to agree on, other than that every thing is addressed :D

Co-authored-by: Andy Kaylor <[email protected]>

erichkeane · 2025-03-12T21:25:05Z

clang/lib/CIR/CodeGen/CIRGenBuilder.h

+                  cir::IntType>(ty))
+      return true;
+
+    assert(0 && "Unexpected MLIR type");


I think the

Suggested change

assert(0 && "Unexpected MLIR type");

assert(!MissingFeatures::unsizedTypes());

was the suggestion here (see the other uses of MissingFeatures).

Okey, i will check MissingFeatures and update it

erichkeane

1 nit, else LGTM.

github-actions · 2025-03-12T22:00:57Z

✅ With the latest revision this PR passed the C/C++ code formatter.

This change adds the basic support for ArrayType Issue llvm#130197

[CIR] Upstream basic support for ArrayType

0b00b1b

AmrDeveloper requested review from AaronBallman, erichkeane, dkolsen-pgi and andykaylor March 9, 2025 18:28

AmrDeveloper requested review from lanza and bcardosolopes as code owners March 9, 2025 18:28

llvmbot added clang Clang issues not falling into any other category ClangIR Anything related to the ClangIR project labels Mar 9, 2025

erichkeane reviewed Mar 10, 2025

View reviewed changes

andykaylor reviewed Mar 10, 2025

View reviewed changes

bcardosolopes reviewed Mar 10, 2025

View reviewed changes

clang/include/clang/CIR/Dialect/IR/CIRTypes.td Show resolved Hide resolved

Add test for LLVM lowering, CIR test and address comments

541da71

andykaylor reviewed Mar 11, 2025

View reviewed changes

Address code review comments

b5cf42b

andykaylor reviewed Mar 11, 2025

View reviewed changes

Update the test to check function name of alloca

6ceabd2

erichkeane reviewed Mar 12, 2025

View reviewed changes

clang/lib/CIR/CodeGen/CIRGenBuilder.h Outdated Show resolved Hide resolved

Update assert message

101f7a0

Co-authored-by: Andy Kaylor <[email protected]>

erichkeane reviewed Mar 12, 2025

View reviewed changes

erichkeane approved these changes Mar 12, 2025

View reviewed changes

AmrDeveloper added 2 commits March 12, 2025 22:33

Add MissingFeatures for unsized types

b386dc1

Merge branch 'main' into cir_upstream_array_type

18163c0

Fix code format after merging

9fb619f

bcardosolopes approved these changes Mar 12, 2025

View reviewed changes

AmrDeveloper merged commit bd0d28a into llvm:main Mar 13, 2025
11 checks passed

frederik-h pushed a commit to frederik-h/llvm-project that referenced this pull request Mar 18, 2025

[CIR] Upstream basic support for ArrayType (llvm#130502)

8e0bd97

This change adds the basic support for ArrayType Issue llvm#130197

		@@ -0,0 +1,12 @@
		// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o - 2>&1 \| FileCheck %s

	auto ty = convertTypeForMemory(converter, dataLayout, type.getEltType());
	mlir::Type ty = convertTypeForMemory(converter, dataLayout, type.getEltType());

	assert(0 && "Unexpected MLIR type");
	assert(!MissingFeatures::unsizedTypes());

[CIR] Upstream basic support for ArrayType #130502

[CIR] Upstream basic support for ArrayType #130502

Uh oh!

Conversation

AmrDeveloper commented Mar 9, 2025

Uh oh!

AmrDeveloper commented Mar 9, 2025

Uh oh!

llvmbot commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmrDeveloper Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andykaylor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andykaylor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AmrDeveloper commented Mar 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Mar 9, 2025 •

edited

Loading

AmrDeveloper Mar 10, 2025 •

edited

Loading

github-actions bot commented Mar 12, 2025 •

edited

Loading