[CIR] Upstream global initialization for ArrayType #131657

AmrDeveloper · 2025-03-17T18:42:27Z

This change adds global initialization for ArrayType

Issue #130197

llvmbot · 2025-03-17T18:43:01Z

@llvm/pr-subscribers-clangir

@llvm/pr-subscribers-clang

Author: Amr Hesham (AmrDeveloper)

Changes

This change adds global initialization for ArrayType

Issue #130197

Patch is 32.77 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/131657.diff

13 Files Affected:

(modified) clang/include/clang/CIR/Dialect/IR/CIRAttrs.td (+42)
(modified) clang/lib/CIR/CodeGen/CIRGenBuilder.h (+34)
(modified) clang/lib/CIR/CodeGen/CIRGenConstantEmitter.h (+4-2)
(modified) clang/lib/CIR/CodeGen/CIRGenDecl.cpp (-1)
(modified) clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp (+112-10)
(modified) clang/lib/CIR/CodeGen/CIRGenModule.cpp (+5-33)
(modified) clang/lib/CIR/Dialect/IR/CIRAttrs.cpp (+109)
(modified) clang/lib/CIR/Dialect/IR/CIRDialect.cpp (+9)
(modified) clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp (+71-4)
(modified) clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.h (+6)
(modified) clang/test/CIR/CodeGen/array.cpp (+20-5)
(modified) clang/test/CIR/IR/array.cir (+18-6)
(modified) clang/test/CIR/Lowering/array.cpp (+23-6)

diff --git a/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td b/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td
index 7b3741de29075..3680ded4afafe 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIRAttrs.td
@@ -154,6 +154,48 @@ def FPAttr : CIR_Attr<"FP", "fp", [TypedAttrInterface]> {
   }];
 }
 
+
+//===----------------------------------------------------------------------===//
+// ConstArrayAttr
+//===----------------------------------------------------------------------===//
+
+def ConstArrayAttr : CIR_Attr<"ConstArray", "const_array", [TypedAttrInterface]> {
+  let summary = "A constant array from ArrayAttr or StringRefAttr";
+  let description = [{
+    An CIR array attribute is an array of literals of the specified attr types.
+  }];
+
+  let parameters = (ins AttributeSelfTypeParameter<"">:$type,
+                        "mlir::Attribute":$elts,
+                        "int":$trailingZerosNum);
+
+  // Define a custom builder for the type; that removes the need to pass
+  // in an MLIRContext instance, as it can be infered from the `type`.
+  let builders = [
+    AttrBuilderWithInferredContext<(ins "cir::ArrayType":$type,
+                                        "mlir::Attribute":$elts), [{
+      int zeros = 0;
+      auto typeSize = mlir::cast<cir::ArrayType>(type).getSize();
+      if (auto str = mlir::dyn_cast<mlir::StringAttr>(elts))
+        zeros = typeSize - str.size();
+      else
+        zeros = typeSize - mlir::cast<mlir::ArrayAttr>(elts).size();
+
+      return $_get(type.getContext(), type, elts, zeros);
+    }]>
+  ];
+
+  // Printing and parsing available in CIRDialect.cpp
+  let hasCustomAssemblyFormat = 1;
+
+  // Enable verifier.
+  let genVerifyDecl = 1;
+
+  let extraClassDeclaration = [{
+    bool hasTrailingZeros() const { return getTrailingZerosNum() != 0; };
+  }];
+}
+
 //===----------------------------------------------------------------------===//
 // ConstPtrAttr
 //===----------------------------------------------------------------------===//
diff --git a/clang/lib/CIR/CodeGen/CIRGenBuilder.h b/clang/lib/CIR/CodeGen/CIRGenBuilder.h
index 260ee25719be1..a2a75148ee884 100644
--- a/clang/lib/CIR/CodeGen/CIRGenBuilder.h
+++ b/clang/lib/CIR/CodeGen/CIRGenBuilder.h
@@ -43,6 +43,40 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
     assert(!cir::MissingFeatures::unsizedTypes());
     return false;
   }
+
+  bool isNullValue(mlir::Attribute attr) const {
+    if (mlir::isa<cir::ZeroAttr>(attr))
+      return true;
+
+    if (const auto ptrVal = mlir::dyn_cast<cir::ConstPtrAttr>(attr))
+      return ptrVal.isNullValue();
+
+    if (const auto intVal = mlir::dyn_cast<cir::IntAttr>(attr))
+      return intVal.isNullValue();
+
+    if (const auto boolVal = mlir::dyn_cast<cir::BoolAttr>(attr))
+      return !boolVal.getValue();
+
+    if (auto fpAttr = mlir::dyn_cast<cir::FPAttr>(attr)) {
+      auto fpVal = fpAttr.getValue();
+      bool ignored;
+      llvm::APFloat fv(+0.0);
+      fv.convert(fpVal.getSemantics(), llvm::APFloat::rmNearestTiesToEven,
+                 &ignored);
+      return fv.bitwiseIsEqual(fpVal);
+    }
+
+    if (const auto arrayVal = mlir::dyn_cast<cir::ConstArrayAttr>(attr)) {
+      if (mlir::isa<mlir::StringAttr>(arrayVal.getElts()))
+        return false;
+      for (const auto elt : mlir::cast<mlir::ArrayAttr>(arrayVal.getElts())) {
+        if (!isNullValue(elt))
+          return false;
+      }
+      return true;
+    }
+    return false;
+  }
 };
 
 } // namespace clang::CIRGen
diff --git a/clang/lib/CIR/CodeGen/CIRGenConstantEmitter.h b/clang/lib/CIR/CodeGen/CIRGenConstantEmitter.h
index 5b22a8e59908d..ca4e607992bbc 100644
--- a/clang/lib/CIR/CodeGen/CIRGenConstantEmitter.h
+++ b/clang/lib/CIR/CodeGen/CIRGenConstantEmitter.h
@@ -20,7 +20,6 @@
 
 #include "CIRGenFunction.h"
 #include "CIRGenModule.h"
-#include "llvm/ADT/SmallVector.h"
 
 namespace clang::CIRGen {
 
@@ -41,6 +40,9 @@ class ConstantEmitter {
   /// block addresses or PredefinedExprs.
   ConstantEmitter(CIRGenFunction &cgf) : cgm(cgf.cgm), cgf(&cgf) {}
 
+  ConstantEmitter(CIRGenModule &cgm, CIRGenFunction *cgf = nullptr)
+      : cgm(cgm), cgf(cgf) {}
+
   ConstantEmitter(const ConstantEmitter &other) = delete;
   ConstantEmitter &operator=(const ConstantEmitter &other) = delete;
 
@@ -66,7 +68,7 @@ class ConstantEmitter {
   mlir::Attribute emitAbstract(SourceLocation loc, const APValue &value,
                                QualType t);
 
-  mlir::Attribute tryEmitConstantExpr(const ConstantExpr *CE);
+  mlir::Attribute tryEmitConstantExpr(const ConstantExpr *ce);
 
   // These are private helper routines of the constant emitter that
   // can't actually be private because things are split out into helper
diff --git a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
index 27ed0113a4f55..a93e8dbcb42de 100644
--- a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
@@ -225,7 +225,6 @@ void CIRGenFunction::emitScalarInit(const Expr *init, mlir::Location loc,
   }
   assert(!cir::MissingFeatures::emitNullabilityCheck());
   emitStoreThroughLValue(RValue::get(value), lvalue, true);
-  return;
 }
 
 void CIRGenFunction::emitExprAsInit(const Expr *init, const ValueDecl *d,
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp b/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp
index 1ea7f6212766c..d3c22f54127c5 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp
@@ -158,13 +158,56 @@ class ConstExprEmitter
 
 // TODO(cir): this can be shared with LLVM's codegen
 static QualType getNonMemoryType(CIRGenModule &cgm, QualType type) {
-  if (auto at = type->getAs<AtomicType>()) {
+  if (const auto *at = type->getAs<AtomicType>()) {
     return cgm.getASTContext().getQualifiedType(at->getValueType(),
                                                 type.getQualifiers());
   }
   return type;
 }
 
+static mlir::Attribute
+emitArrayConstant(CIRGenModule &cgm, mlir::Type desiredType,
+                  mlir::Type commonElementType, unsigned arrayBound,
+                  SmallVectorImpl<mlir::TypedAttr> &elements,
+                  mlir::TypedAttr filter) {
+  const auto &builder = cgm.getBuilder();
+
+  unsigned nonzeroLength = arrayBound;
+  if (elements.size() < nonzeroLength && builder.isNullValue(filter))
+    nonzeroLength = elements.size();
+
+  if (nonzeroLength == elements.size()) {
+    while (nonzeroLength > 0 &&
+           builder.isNullValue(elements[nonzeroLength - 1]))
+      --nonzeroLength;
+  }
+
+  if (nonzeroLength == 0)
+    return cir::ZeroAttr::get(builder.getContext(), desiredType);
+
+  const unsigned trailingZeroes = arrayBound - nonzeroLength;
+  if (trailingZeroes >= 8) {
+    if (elements.size() < nonzeroLength)
+      cgm.errorNYI("missing initializer for non-zero element");
+  } else if (elements.size() != arrayBound) {
+    elements.resize(arrayBound, filter);
+
+    if (filter.getType() != commonElementType)
+      cgm.errorNYI(
+          "array filter type should always be the same as element type");
+  }
+
+  SmallVector<mlir::Attribute, 4> eles;
+  eles.reserve(elements.size());
+
+  for (const auto &element : elements)
+    eles.push_back(element);
+
+  return cir::ConstArrayAttr::get(
+      cir::ArrayType::get(builder.getContext(), commonElementType, arrayBound),
+      mlir::ArrayAttr::get(builder.getContext(), eles));
+}
+
 //===----------------------------------------------------------------------===//
 //                             ConstantEmitter
 //===----------------------------------------------------------------------===//
@@ -271,16 +314,61 @@ mlir::Attribute ConstantEmitter::tryEmitPrivate(const APValue &value,
         cgm.getASTContext().getTargetInfo().useFP16ConversionIntrinsics()) {
       cgm.errorNYI("ConstExprEmitter::tryEmitPrivate half");
       return {};
-    } else {
-      mlir::Type ty = cgm.convertType(destType);
-      assert(mlir::isa<cir::CIRFPTypeInterface>(ty) &&
-             "expected floating-point type");
-      return cgm.getBuilder().getAttr<cir::FPAttr>(ty, init);
     }
+
+    mlir::Type ty = cgm.convertType(destType);
+    assert(mlir::isa<cir::CIRFPTypeInterface>(ty) &&
+           "expected floating-point type");
+    return cgm.getBuilder().getAttr<cir::FPAttr>(ty, init);
   }
   case APValue::Array: {
-    cgm.errorNYI("ConstExprEmitter::tryEmitPrivate array");
-    return {};
+    const ArrayType *arrayTy = cgm.getASTContext().getAsArrayType(destType);
+    const QualType arrayElementTy = arrayTy->getElementType();
+    const unsigned numElements = value.getArraySize();
+    const unsigned numInitElts = value.getArrayInitializedElts();
+
+    mlir::Attribute filter;
+    if (value.hasArrayFiller()) {
+      filter =
+          tryEmitPrivate(value.getArrayFiller(), arrayTy->getElementType());
+      if (!filter)
+        return {};
+    }
+
+    SmallVector<mlir::TypedAttr, 16> elements;
+    if (filter && builder.isNullValue(filter))
+      elements.reserve(numInitElts + 1);
+    else
+      elements.reserve(numInitElts);
+
+    mlir::Type commonElementType;
+    for (unsigned i = 0; i < numInitElts; ++i) {
+      const APValue &arrayElement = value.getArrayInitializedElt(i);
+      const mlir::Attribute element =
+          tryEmitPrivateForMemory(arrayElement, arrayElementTy);
+      if (!element)
+        return {};
+
+      const mlir::TypedAttr elementTyped = mlir::cast<mlir::TypedAttr>(element);
+      if (i == 0)
+        commonElementType = elementTyped.getType();
+      else if (elementTyped.getType() != commonElementType) {
+        cgm.errorNYI("ConstExprEmitter::tryEmitPrivate Array without common "
+                     "element type");
+        return {};
+      }
+
+      elements.push_back(elementTyped);
+    }
+
+    mlir::TypedAttr typedFilter =
+        llvm::dyn_cast_or_null<mlir::TypedAttr>(filter);
+    if (filter && !typedFilter)
+      cgm.errorNYI("array filter should always be typed");
+
+    mlir::Type desiredType = cgm.convertType(destType);
+    return emitArrayConstant(cgm, desiredType, commonElementType, numElements,
+                             elements, typedFilter);
   }
   case APValue::Vector: {
     cgm.errorNYI("ConstExprEmitter::tryEmitPrivate vector");
@@ -290,9 +378,23 @@ mlir::Attribute ConstantEmitter::tryEmitPrivate(const APValue &value,
     cgm.errorNYI("ConstExprEmitter::tryEmitPrivate member pointer");
     return {};
   }
-  case APValue::LValue:
-    cgm.errorNYI("ConstExprEmitter::tryEmitPrivate lvalue");
+  case APValue::LValue: {
+
+    if (value.getLValueBase()) {
+      cgm.errorNYI("non-null pointer initialization");
+    } else {
+
+      mlir::Type desiredType = cgm.convertType(destType);
+      if (const cir::PointerType ptrType =
+              mlir::dyn_cast<cir::PointerType>(desiredType)) {
+        return builder.getConstPtrAttr(ptrType,
+                                       value.getLValueOffset().getQuantity());
+      } else {
+        llvm_unreachable("non-pointer variable initialized with a pointer");
+      }
+    }
     return {};
+  }
   case APValue::Struct:
   case APValue::Union:
     cgm.errorNYI("ConstExprEmitter::tryEmitPrivate struct or union");
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
index 0e3e15ca2cadc..c7620a8355b2f 100644
--- a/clang/lib/CIR/CodeGen/CIRGenModule.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
@@ -11,6 +11,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "CIRGenModule.h"
+#include "CIRGenConstantEmitter.h"
 #include "CIRGenFunction.h"
 
 #include "clang/AST/ASTContext.h"
@@ -127,7 +128,8 @@ void CIRGenModule::emitGlobalFunctionDefinition(clang::GlobalDecl gd,
 
 void CIRGenModule::emitGlobalVarDefinition(const clang::VarDecl *vd,
                                            bool isTentative) {
-  mlir::Type type = convertType(vd->getType());
+  const QualType astTy = vd->getType();
+  const mlir::Type type = convertType(vd->getType());
   if (clang::IdentifierInfo *identifier = vd->getIdentifier()) {
     auto varOp = builder.create<cir::GlobalOp>(getLoc(vd->getSourceRange()),
                                                identifier->getName(), type);
@@ -140,38 +142,8 @@ void CIRGenModule::emitGlobalVarDefinition(const clang::VarDecl *vd,
     if (initExpr) {
       mlir::Attribute initializer;
       if (APValue *value = initDecl->evaluateValue()) {
-        switch (value->getKind()) {
-        case APValue::Int: {
-          if (mlir::isa<cir::BoolType>(type))
-            initializer =
-                builder.getCIRBoolAttr(value->getInt().getZExtValue());
-          else
-            initializer = builder.getAttr<cir::IntAttr>(type, value->getInt());
-          break;
-        }
-        case APValue::Float: {
-          initializer = builder.getAttr<cir::FPAttr>(type, value->getFloat());
-          break;
-        }
-        case APValue::LValue: {
-          if (value->getLValueBase()) {
-            errorNYI(initExpr->getSourceRange(),
-                     "non-null pointer initialization");
-          } else {
-            if (auto ptrType = mlir::dyn_cast<cir::PointerType>(type)) {
-              initializer = builder.getConstPtrAttr(
-                  ptrType, value->getLValueOffset().getQuantity());
-            } else {
-              llvm_unreachable(
-                  "non-pointer variable initialized with a pointer");
-            }
-          }
-          break;
-        }
-        default:
-          errorNYI(initExpr->getSourceRange(), "unsupported initializer kind");
-          break;
-        }
+        ConstantEmitter emitter(*this);
+        initializer = emitter.tryEmitPrivateForMemory(*value, astTy);
       } else {
         errorNYI(initExpr->getSourceRange(), "non-constant initializer");
       }
diff --git a/clang/lib/CIR/Dialect/IR/CIRAttrs.cpp b/clang/lib/CIR/Dialect/IR/CIRAttrs.cpp
index 8e8f7d5b7d7cb..8dfe56b75a47b 100644
--- a/clang/lib/CIR/Dialect/IR/CIRAttrs.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRAttrs.cpp
@@ -190,6 +190,115 @@ LogicalResult FPAttr::verify(function_ref<InFlightDiagnostic()> emitError,
   return success();
 }
 
+//===----------------------------------------------------------------------===//
+// CIR ConstArrayAttr
+//===----------------------------------------------------------------------===//
+
+LogicalResult
+ConstArrayAttr::verify(function_ref<::mlir::InFlightDiagnostic()> emitError,
+                       Type type, Attribute elts, int trailingZerosNum) {
+
+  if (!(mlir::isa<ArrayAttr>(elts) || mlir::isa<StringAttr>(elts)))
+    return emitError() << "constant array expects ArrayAttr or StringAttr";
+
+  if (StringAttr strAttr = mlir::dyn_cast<StringAttr>(elts)) {
+    ArrayType arrayTy = mlir::cast<ArrayType>(type);
+    IntType intTy = mlir::dyn_cast<IntType>(arrayTy.getEltType());
+
+    // TODO: add CIR type for char.
+    if (!intTy || intTy.getWidth() != 8) {
+      emitError() << "constant array element for string literals expects "
+                     "!cir.int<u, 8> element type";
+      return failure();
+    }
+    return success();
+  }
+
+  assert(mlir::isa<ArrayAttr>(elts));
+  ArrayAttr arrayAttr = mlir::cast<mlir::ArrayAttr>(elts);
+  ArrayType arrayTy = mlir::cast<ArrayType>(type);
+
+  // Make sure both number of elements and subelement types match type.
+  if (arrayTy.getSize() != arrayAttr.size() + trailingZerosNum)
+    return emitError() << "constant array size should match type size";
+  return success();
+}
+
+Attribute ConstArrayAttr::parse(AsmParser &parser, Type type) {
+  ::mlir::FailureOr<Type> resultTy;
+  ::mlir::FailureOr<Attribute> resultVal;
+  ::llvm::SMLoc loc = parser.getCurrentLocation();
+  (void)loc;
+  // Parse literal '<'
+  if (parser.parseLess())
+    return {};
+
+  // Parse variable 'value'
+  resultVal = FieldParser<Attribute>::parse(parser);
+  if (failed(resultVal)) {
+    parser.emitError(
+        parser.getCurrentLocation(),
+        "failed to parse ConstArrayAttr parameter 'value' which is "
+        "to be a `Attribute`");
+    return {};
+  }
+
+  // ArrayAttrrs have per-element type, not the type of the array...
+  if (mlir::dyn_cast<ArrayAttr>(*resultVal)) {
+    // Array has implicit type: infer from const array type.
+    if (parser.parseOptionalColon().failed()) {
+      resultTy = type;
+    } else { // Array has explicit type: parse it.
+      resultTy = FieldParser<Type>::parse(parser);
+      if (failed(resultTy)) {
+        parser.emitError(
+            parser.getCurrentLocation(),
+            "failed to parse ConstArrayAttr parameter 'type' which is "
+            "to be a `::mlir::Type`");
+        return {};
+      }
+    }
+  } else {
+    assert(mlir::isa<TypedAttr>(*resultVal) && "IDK");
+    auto ta = mlir::cast<TypedAttr>(*resultVal);
+    resultTy = ta.getType();
+    if (mlir::isa<mlir::NoneType>(*resultTy)) {
+      parser.emitError(parser.getCurrentLocation(),
+                       "expected type declaration for string literal");
+      return {};
+    }
+  }
+
+  auto zeros = 0;
+  if (parser.parseOptionalComma().succeeded()) {
+    if (parser.parseOptionalKeyword("trailing_zeros").succeeded()) {
+      auto typeSize = mlir::cast<cir::ArrayType>(resultTy.value()).getSize();
+      auto elts = resultVal.value();
+      if (auto str = mlir::dyn_cast<mlir::StringAttr>(elts))
+        zeros = typeSize - str.size();
+      else
+        zeros = typeSize - mlir::cast<mlir::ArrayAttr>(elts).size();
+    } else {
+      return {};
+    }
+  }
+
+  // Parse literal '>'
+  if (parser.parseGreater())
+    return {};
+
+  return parser.getChecked<ConstArrayAttr>(
+      loc, parser.getContext(), resultTy.value(), resultVal.value(), zeros);
+}
+
+void ConstArrayAttr::print(AsmPrinter &printer) const {
+  printer << "<";
+  printer.printStrippedAttrOrType(getElts());
+  if (getTrailingZerosNum())
+    printer << ", trailing_zeros";
+  printer << ">";
+}
+
 //===----------------------------------------------------------------------===//
 // CIR Dialect
 //===----------------------------------------------------------------------===//
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index d041791770d82..467e6237ef01a 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -149,6 +149,12 @@ static LogicalResult checkConstantTypes(mlir::Operation *op, mlir::Type opType,
     return success();
   }
 
+  if (isa<cir::ZeroAttr>(attrType)) {
+    if (::mlir::isa<cir::ArrayType>(opType))
+      return success();
+    return op->emitOpError("zero expects struct or array type");
+  }
+
   if (mlir::isa<cir::BoolAttr>(attrType)) {
     if (!mlir::isa<cir::BoolType>(opType))
       return op->emitOpError("result type (")
@@ -166,6 +172,9 @@ static LogicalResult checkConstantTypes(mlir::Operation *op, mlir::Type opType,
     return success();
   }
 
+  if (mlir::isa<cir::ConstArrayAttr>(attrType))
+    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/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
index 0cd27ecf1a3bd..f0b9986a9efaf 100644
--- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
+++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
@@ -113,6 +113,21 @@ static mlir::Value emitToMemory(mlir::ConversionPatternRewriter &rewriter,
   return value;
 }
 
+static mlir::Value
+emitCirAttrToMemory(mlir::Operation *parentOp, mlir::Attribute attr,
+                    mlir::ConversionPatternRewriter &rewriter,
+                    const mlir::TypeConverter *converter,
+                    mlir::DataLayout const &dataLayout) {
+
+  mlir::Value loweredValue =
+      lowerCirAttrAsValue(parentOp, attr, rewriter, converter);
+  if (auto boolAttr = mlir::dyn_cast<cir::BoolAttr>(attr)) {
+    return emitToMemory(rewriter, dataLayout, boolAttr.getType(), loweredValue);
+  }
+
+  return loweredValue;
+}
+
 mlir::LLVM::Linkage convertLinkage(cir::GlobalLinkageKind linkage) {
   using CIR = cir::GlobalLinkageKind;
   using LLVM = mlir::LLVM::Linkage;
@@ -151,14 +166,1...
[truncated]

erichkeane

I did a quick look, I'll count on the CIR reviewers to do a closer look.

andykaylor · 2025-03-17T22:25:12Z

clang/lib/CIR/CodeGen/CIRGenBuilder.h

@@ -43,6 +43,40 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
    assert(!cir::MissingFeatures::unsizedTypes());
    return false;
  }
+
+  bool isNullValue(mlir::Attribute attr) const {


It would be useful to have a comment here explaining the intent of this function. In particular, some clarification is necessary as to what it means for an FPAtt. The function returns false for -0.0, which means it is not simply testing for equality with zero.

andykaylor · 2025-03-17T22:33:58Z

clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp

+                  mlir::Type commonElementType, unsigned arrayBound,
+                  SmallVectorImpl<mlir::TypedAttr> &elements,
+                  mlir::TypedAttr filter) {
+  const auto &builder = cgm.getBuilder();


Don't use auto here.

andykaylor · 2025-03-17T22:41:32Z

clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp

+    return cir::ZeroAttr::get(builder.getContext(), desiredType);
+
+  const unsigned trailingZeroes = arrayBound - nonzeroLength;
+  if (trailingZeroes >= 8) {


You've dropped a comment from the incubator code here that explains the purpose of the comparison with 8 -- "if we have lots of trailing zeroes". Without the comment, 8 is purely a magic number with no context.

andykaylor · 2025-03-17T22:44:06Z

clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp

+  const unsigned trailingZeroes = arrayBound - nonzeroLength;
+  if (trailingZeroes >= 8) {
+    if (elements.size() < nonzeroLength)
+      cgm.errorNYI("missing initializer for non-zero element");


I don't think errorNYI is appropriate here. The assertion in the incubator is asserting a condition that should always be true. It's not a missing implementation detail. I see that you have omitted the handling for the zeroinitializer array filler, which is fine. The errorNYI call for that should be separate from the comparison here.

andykaylor · 2025-03-17T22:47:57Z

clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp

+emitArrayConstant(CIRGenModule &cgm, mlir::Type desiredType,
+                  mlir::Type commonElementType, unsigned arrayBound,
+                  SmallVectorImpl<mlir::TypedAttr> &elements,
+                  mlir::TypedAttr filter) {


It looks like "filler" got changed to "filter" here in the transition from the incubator code. That's something very different.

andykaylor · 2025-03-17T23:23:40Z

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

+    }
+  }
+
+  auto zeros = 0;


Suggested change

auto zeros = 0;

unsigned zeros = 0;

andykaylor · 2025-03-17T23:24:47Z

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

+  if (parser.parseOptionalComma().succeeded()) {
+    if (parser.parseOptionalKeyword("trailing_zeros").succeeded()) {
+      auto typeSize = mlir::cast<cir::ArrayType>(resultTy.value()).getSize();
+      auto elts = resultVal.value();


Suggested change

auto elts = resultVal.value();

mlir::Attribute elts = resultVal.value();

andykaylor · 2025-03-17T23:27:06Z

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

+  auto zeros = 0;
+  if (parser.parseOptionalComma().succeeded()) {
+    if (parser.parseOptionalKeyword("trailing_zeros").succeeded()) {
+      auto typeSize = mlir::cast<cir::ArrayType>(resultTy.value()).getSize();


Suggested change

auto typeSize = mlir::cast<cir::ArrayType>(resultTy.value()).getSize();

unsigned typeSize = mlir::cast<cir::ArrayType>(resultTy.value()).getSize();

andykaylor · 2025-03-17T23:29:00Z

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

+                                mlir::ConversionPatternRewriter &rewriter,
+                                const mlir::TypeConverter *converter) {
+  CIRAttrToValue valueConverter(parentOp, rewriter, converter);
+  auto value = valueConverter.visit(attr);


Suggested change

auto value = valueConverter.visit(attr);

mlir::Value value = valueConverter.visit(attr);

andykaylor · 2025-03-17T23:38:11Z

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

+    for (auto [idx, elt] : llvm::enumerate(arrayAttr)) {
+      mlir::DataLayout dataLayout(parentOp->getParentOfType<mlir::ModuleOp>());
+      mlir::Value init =
+          emitCirAttrToMemory(parentOp, elt, rewriter, converter, dataLayout);


I think you can just call visit(elt) here. The extra wrapper calls shouldn't be needed.

AmrDeveloper · 2025-03-18T17:59:05Z

@andykaylor All comments addressed except two points related to NYI

AmrDeveloper · 2025-03-18T18:51:13Z

CI is red because unrelated error

fatal: Could not read from remote repository.
 
Please make sure you have the correct access rights

github-actions · 2025-03-18T19:28:09Z

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

bcardosolopes

I have nothing to add on top of Andy's review, LGTM once he's happy with the final state.

erichkeane

Happy when Andy is as well!

andykaylor

I have just a few more issues.

andykaylor · 2025-03-19T17:04:39Z

clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp

+    elements.resize(arrayBound, filter);
+
+    if (filter.getType() != commonElementType)
+      cgm.errorNYI(


I see. It confused me because you put the errorNYI here rather than at the bottom of the function where the incubator code creates the struct. I suppose this way would work, but it will be easier to update the code later if you maintain the structure of the incubator code and put the errorNYI code at the end.

andykaylor · 2025-03-19T17:09:38Z

clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp

+      if (i == 0)
+        commonElementType = elementTyped.getType();
+      else if (elementTyped.getType() != commonElementType) {
+        cgm.errorNYI("ConstExprEmitter::tryEmitPrivate Array without common "


If you just set commonElementType to {} as is done in the incubator, this will eventually reach the NYI state in emitConstantArray and this code won't need to be updated later.

andykaylor · 2025-03-19T17:18:24Z

clang/lib/CIR/CodeGen/CIRGenBuilder.h

@@ -45,6 +45,41 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
    return false;
  }

+  // Return true if this is the null value


This isn't quite the comment I wanted to see. It doesn't tell me any more than the function name did. It needs to explain what "null value" means in the context of this function. Specifically, it doesn't mean "is equal to zero" because -0.0 is equal to zero. Strings also get special handling, as will structures when they are implemented. Please describe the purpose of the function and highlight the special cases.

That's right, I updated the comment to describe the implemented cases

andykaylor · 2025-03-19T17:25:27Z

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

@@ -228,6 +257,42 @@ mlir::Value CIRAttrToValue::visitCirAttr(cir::FPAttr fltAttr) {
      loc, converter->convertType(fltAttr.getType()), fltAttr.getValue());
 }

+// ConstArrayAttr visitor
+mlir::Value CIRAttrToValue::visitCirAttr(cir::ConstArrayAttr attr) {
+  auto llvmTy = converter->convertType(attr.getType());


Don't use auto here. The same applies for loc and arrayTy below.

AmrDeveloper · 2025-03-19T19:10:49Z

If you just set commonElementType to {} as is done in the incubator, this will eventually reach the NYI state in emitConstantArray and this code won't need to be updated later.

@andykaylor Yes, this is a better approach, I updated the code

Thanks

andykaylor

Looks good. Thanks for the updates!

andykaylor · 2025-03-19T20:05:04Z

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

+
+/// ZeroAttr visitor.
+mlir::Value CIRAttrToValue::visitCirAttr(cir::ZeroAttr attr) {
+  auto loc = parentOp->getLoc();


You missed one auto here. Feel free to merge this now and submit a follow-up to fix this. I'm waiting for this PR to be merged so I can rebase my type alias change without breaking tests for PRs that are ready to be merged.

Okay, once CI is green I will merge it and create a follow-up fix for this auto

Thanks

Follow up PR to address style comment (#131657 (comment))

Follow up PR to address style comment (llvm/llvm-project#131657 (comment))

[CIR] Upstream global initialization for ArrayType

4ae9c42

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

AmrDeveloper requested review from lanza and bcardosolopes as code owners March 17, 2025 18:42

llvmbot added clang ClangIR labels Mar 17, 2025

erichkeane reviewed Mar 17, 2025

View reviewed changes

andykaylor reviewed Mar 17, 2025

View reviewed changes

Address code review comments

ea0aee4

Merge branch 'main' into cir_upstream_array_init

9cbe2e4

Merge branch 'main' into cir_upstream_array_init

27eaf94

Format the code

4c97120

AmrDeveloper force-pushed the cir_upstream_array_init branch from c67328b to 4c97120 Compare March 18, 2025 20:15

bcardosolopes approved these changes Mar 19, 2025

View reviewed changes

erichkeane approved these changes Mar 19, 2025

View reviewed changes

andykaylor reviewed Mar 19, 2025

View reviewed changes

Address code review comments

978303c

andykaylor approved these changes Mar 19, 2025

View reviewed changes

andykaylor reviewed Mar 19, 2025

View reviewed changes

AmrDeveloper merged commit 6aeae62 into llvm:main Mar 19, 2025
7 of 10 checks passed

AmrDeveloper mentioned this pull request Mar 19, 2025

[CIR] Follow up for addressing style comment #132093

Merged

AmrDeveloper added a commit that referenced this pull request Mar 20, 2025

[CIR] Follow up for addressing style comment (#132093)

7c64790

Follow up PR to address style comment (#131657 (comment))

llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Mar 20, 2025

Automerge: [CIR] Follow up for addressing style comment (#132093)

9871d72

Follow up PR to address style comment (llvm/llvm-project#131657 (comment))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIR] Upstream global initialization for ArrayType #131657

[CIR] Upstream global initialization for ArrayType #131657

AmrDeveloper commented Mar 17, 2025

llvmbot commented Mar 17, 2025 •

edited

Loading

erichkeane left a comment

andykaylor Mar 17, 2025

andykaylor Mar 17, 2025

andykaylor Mar 17, 2025

andykaylor Mar 17, 2025

andykaylor Mar 17, 2025

andykaylor Mar 17, 2025

andykaylor Mar 17, 2025

andykaylor Mar 17, 2025

andykaylor Mar 17, 2025

andykaylor Mar 17, 2025

AmrDeveloper commented Mar 18, 2025

AmrDeveloper commented Mar 18, 2025

github-actions bot commented Mar 18, 2025 •

edited

Loading

bcardosolopes left a comment

erichkeane left a comment

andykaylor left a comment

andykaylor Mar 19, 2025

andykaylor Mar 19, 2025

andykaylor Mar 19, 2025

AmrDeveloper Mar 19, 2025

andykaylor Mar 19, 2025

AmrDeveloper commented Mar 19, 2025

andykaylor left a comment

andykaylor Mar 19, 2025

AmrDeveloper Mar 19, 2025

	auto elts = resultVal.value();
	mlir::Attribute elts = resultVal.value();

	auto typeSize = mlir::cast<cir::ArrayType>(resultTy.value()).getSize();
	unsigned typeSize = mlir::cast<cir::ArrayType>(resultTy.value()).getSize();

	auto value = valueConverter.visit(attr);
	mlir::Value value = valueConverter.visit(attr);

[CIR] Upstream global initialization for ArrayType #131657

[CIR] Upstream global initialization for ArrayType #131657

Conversation

AmrDeveloper commented Mar 17, 2025

llvmbot commented Mar 17, 2025 • edited Loading

erichkeane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmrDeveloper commented Mar 18, 2025

AmrDeveloper commented Mar 18, 2025

github-actions bot commented Mar 18, 2025 • edited Loading

bcardosolopes left a comment

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment

andykaylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmrDeveloper commented Mar 19, 2025

andykaylor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llvmbot commented Mar 17, 2025 •

edited

Loading

github-actions bot commented Mar 18, 2025 •

edited

Loading