Skip to content

[CIR] Upstream basic support for ArrayType #130502

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 13, 2025
Merged

[CIR] Upstream basic support for ArrayType #130502

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0b00b1b
[CIR] Upstream basic support for ArrayType
AmrDeveloper Mar 9, 2025
541da71
Add test for LLVM lowering, CIR test and address comments
AmrDeveloper Mar 10, 2025
b5cf42b
Address code review comments
AmrDeveloper Mar 11, 2025
6ceabd2
Update the test to check function name of alloca
AmrDeveloper Mar 11, 2025
101f7a0
Update assert message
AmrDeveloper Mar 12, 2025
b386dc1
Add MissingFeatures for unsized types
AmrDeveloper Mar 12, 2025
18163c0
Merge branch 'main' into cir_upstream_array_type
AmrDeveloper Mar 12, 2025
9fb619f
Fix code format after merging
AmrDeveloper Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions clang/include/clang/CIR/Dialect/IR/CIRAttrs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
23 changes: 21 additions & 2 deletions clang/include/clang/CIR/Dialect/IR/CIRTypes.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -386,8 +405,8 @@ def VoidPtr : Type<
//===----------------------------------------------------------------------===//

def CIR_AnyType : AnyTypeOf<[
CIR_VoidType, CIR_BoolType, CIR_IntType, CIR_AnyFloat, CIR_PointerType,
CIR_FuncType
CIR_VoidType, CIR_BoolType, CIR_ArrayType, CIR_IntType, CIR_AnyFloat,
CIR_PointerType, CIR_FuncType
]>;

#endif // MLIR_CIR_DIALECT_CIR_TYPES
5 changes: 5 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor

@andykaylor andykaylor Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@AmrDeveloper AmrDeveloper Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cir::IntType>(ty);
}
};

} // namespace clang::CIRGen
Expand Down
15 changes: 15 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,21 @@ mlir::Type CIRGenTypes::convertType(QualType type) {
break;
}

case Type::ConstantArray: {
const ConstantArrayType *arrTy = cast<ConstantArrayType>(ty);
mlir::Type elemTy = convertTypeForMem(arrTy->getElementType());

if (!builder.isSized(elemTy)) {
cgm.errorNYI(SourceLocation(), "unimplemented size for type", type);
Copy link
Contributor

@andykaylor andykaylor Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

resultType = cir::ArrayType::get(builder.getContext(), cgm.SInt32Ty, 0);
break;
}

resultType = cir::ArrayType::get(builder.getContext(), elemTy,
arrTy->getSize().getZExtValue());
break;
}

case Type::FunctionNoProto:
case Type::FunctionProto:
resultType = convertFunctionTypeInternal(type);
Expand Down
16 changes: 16 additions & 0 deletions clang/lib/CIR/Dialect/IR/CIRTypes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,22 @@ BoolType::getABIAlignment(const ::mlir::DataLayout &dataLayout,
return 1;
}

//===----------------------------------------------------------------------===//
// Definitions
//===----------------------------------------------------------------------===//

llvm::TypeSize
ArrayType::getTypeSizeInBits(const ::mlir::DataLayout &dataLayout,
Copy link
Contributor

@andykaylor andykaylor Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@AmrDeveloper AmrDeveloper Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@andykaylor andykaylor Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@AmrDeveloper AmrDeveloper Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@andykaylor andykaylor Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for the clarification.

::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
//===----------------------------------------------------------------------===//
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Copy link
Contributor

@andykaylor andykaylor Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return mlir::LLVM::LLVMArrayType::get(ty, type.getSize());
});
converter.addConversion([&](cir::BoolType type) -> mlir::Type {
return mlir::IntegerType::get(type.getContext(), 1,
mlir::IntegerType::Signless);
Expand Down
24 changes: 24 additions & 0 deletions clang/test/CIR/CodeGen/array.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o - 2>&1 | FileCheck %s
Copy link
Contributor

@andykaylor andykaylor Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


int a[10];
// 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>
Copy link
Contributor

@andykaylor andykaylor Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

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

void f() {
int l[10];
// CHECK: %[[ARR:.*]] = cir.alloca !cir.array<!cir.int<s, 32> x 10>, !cir.ptr<!cir.array<!cir.int<s, 32> x 10>>, ["l"]
}

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

@andykaylor andykaylor Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


void f3(int pp[10][10]) {}
// CHECK: cir.func @f3(%arg0: !cir.ptr<!cir.array<!cir.int<s, 32> x 10>>
51 changes: 51 additions & 0 deletions clang/test/CIR/IR/array.cir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// RUN: cir-opt %s | FileCheck %s

module {

cir.global external @a : !cir.array<!cir.int<s, 32> x 10>
// CHECK: cir.global external @a : !cir.array<!cir.int<s, 32> x 10>

cir.global external @aa : !cir.array<!cir.array<!cir.int<s, 32> x 10> x 10>
// CHECK: cir.global external @aa : !cir.array<!cir.array<!cir.int<s, 32> x 10> x 10>

cir.global external @b : !cir.array<!cir.int<s, 32> x 10>
// CHECK: cir.global external @b : !cir.array<!cir.int<s, 32> x 10>

cir.global external @bb : !cir.array<!cir.array<!cir.int<s, 32> x 10> x 10>
// CHECK: cir.global external @bb : !cir.array<!cir.array<!cir.int<s, 32> x 10> x 10>

cir.func @f() {
%0 = cir.alloca !cir.array<!cir.int<s, 32> x 10>, !cir.ptr<!cir.array<!cir.int<s, 32> x 10>>, ["l"] {alignment = 4 : i64}
cir.return
}

// CHECK: cir.func @f() {
// CHECK: %0 = cir.alloca !cir.array<!cir.int<s, 32> x 10>, !cir.ptr<!cir.array<!cir.int<s, 32> x 10>>, ["l"] {alignment = 4 : i64}
// CHECK: cir.return
// CHECK: }

cir.func @f2(%arg0: !cir.ptr<!cir.int<s, 32>>) {
%0 = cir.alloca !cir.ptr<!cir.int<s, 32>>, !cir.ptr<!cir.ptr<!cir.int<s, 32>>>, ["p", init] {alignment = 8 : i64}
cir.store %arg0, %0 : !cir.ptr<!cir.int<s, 32>>, !cir.ptr<!cir.ptr<!cir.int<s, 32>>>
cir.return
}

// CHECK: cir.func @f2(%arg0: !cir.ptr<!cir.int<s, 32>>) {
// CHECK: %0 = cir.alloca !cir.ptr<!cir.int<s, 32>>, !cir.ptr<!cir.ptr<!cir.int<s, 32>>>, ["p", init] {alignment = 8 : i64}
// CHECK: cir.store %arg0, %0 : !cir.ptr<!cir.int<s, 32>>, !cir.ptr<!cir.ptr<!cir.int<s, 32>>>
// CHECK: cir.return
// CHECK: }

cir.func @f3(%arg0: !cir.ptr<!cir.array<!cir.int<s, 32> x 10>>) {
%0 = cir.alloca !cir.ptr<!cir.array<!cir.int<s, 32> x 10>>, !cir.ptr<!cir.ptr<!cir.array<!cir.int<s, 32> x 10>>>, ["pp", init] {alignment = 8 : i64}
cir.store %arg0, %0 : !cir.ptr<!cir.array<!cir.int<s, 32> x 10>>, !cir.ptr<!cir.ptr<!cir.array<!cir.int<s, 32> x 10>>>
cir.return
}

// CHECK: cir.func @f3(%arg0: !cir.ptr<!cir.array<!cir.int<s, 32> x 10>>) {
// CHECK: %0 = cir.alloca !cir.ptr<!cir.array<!cir.int<s, 32> x 10>>, !cir.ptr<!cir.ptr<!cir.array<!cir.int<s, 32> x 10>>>, ["pp", init] {alignment = 8 : i64}
// CHECK: cir.store %arg0, %0 : !cir.ptr<!cir.array<!cir.int<s, 32> x 10>>, !cir.ptr<!cir.ptr<!cir.array<!cir.int<s, 32> x 10>>>
// CHECK: cir.return
// CHECK: }

}
24 changes: 24 additions & 0 deletions clang/test/CIR/Lowering/array.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o - 2>&1 | FileCheck %s

int a[10];
// CHECK: @a = external dso_local global [10 x i32]

int aa[10][10];
// CHECK: @aa = external dso_local global [10 x [10 x i32]]

extern int b[10];
// CHECK: @b = external dso_local global [10 x i32]

extern int bb[10][10];
// CHECK: @bb = external dso_local global [10 x [10 x i32]]

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

@andykaylor andykaylor Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


void f2(int p[10]) {}
// CHECK: alloca ptr, i64 1, align 8

void f3(int pp[10][10]) {}
// CHECK: alloca ptr, i64 1, align 8