Address code review comments

Author: AmrDeveloper

clang/lib/CIR/CodeGen/CIRGenBuilder.h

@@ -44,6 +44,7 @@ class CIRGenBuilderTy : public cir::CIRBaseBuilderTy {
     return false;
   }
 
+  // Return true if this is the null value
   bool isNullValue(mlir::Attribute attr) const {
     if (mlir::isa<cir::ZeroAttr>(attr))
       return true;

clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp

@@ -169,11 +169,11 @@ 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();
+                  mlir::TypedAttr filler) {
+  const CIRGenBuilderTy &builder = cgm.getBuilder();
 
   unsigned nonzeroLength = arrayBound;
-  if (elements.size() < nonzeroLength && builder.isNullValue(filter))
+  if (elements.size() < nonzeroLength && builder.isNullValue(filler))
     nonzeroLength = elements.size();
 
   if (nonzeroLength == elements.size()) {
@@ -186,13 +186,15 @@ emitArrayConstant(CIRGenModule &cgm, mlir::Type desiredType,
     return cir::ZeroAttr::get(builder.getContext(), desiredType);
 
   const unsigned trailingZeroes = arrayBound - nonzeroLength;
+
+  // Add a zeroinitializer array filler if we have lots of trailing zeroes.
   if (trailingZeroes >= 8) {
-    if (elements.size() < nonzeroLength)
-      cgm.errorNYI("missing initializer for non-zero element");
+    assert(elements.size() >= nonzeroLength &&
+           "missing initializer for non-zero element");
   } else if (elements.size() != arrayBound) {
-    elements.resize(arrayBound, filter);
+    elements.resize(arrayBound, filler);
 
-    if (filter.getType() != commonElementType)
+    if (filler.getType() != commonElementType)
       cgm.errorNYI(
           "array filter type should always be the same as element type");
   }
@@ -327,16 +329,16 @@ mlir::Attribute ConstantEmitter::tryEmitPrivate(const APValue &value,
     const unsigned numElements = value.getArraySize();
     const unsigned numInitElts = value.getArrayInitializedElts();
 
-    mlir::Attribute filter;
+    mlir::Attribute filler;
     if (value.hasArrayFiller()) {
-      filter =
-          tryEmitPrivate(value.getArrayFiller(), arrayTy->getElementType());
-      if (!filter)
+      filler =
+          tryEmitPrivate(value.getArrayFiller(), arrayElementTy);
+      if (!filler)
         return {};
     }
 
     SmallVector<mlir::TypedAttr, 16> elements;
-    if (filter && builder.isNullValue(filter))
+    if (filler && builder.isNullValue(filler))
       elements.reserve(numInitElts + 1);
     else
       elements.reserve(numInitElts);
@@ -361,14 +363,14 @@ mlir::Attribute ConstantEmitter::tryEmitPrivate(const APValue &value,
       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::TypedAttr typedFiller =
+        llvm::cast_or_null<mlir::TypedAttr>(filler);
+    if (filler && !typedFiller)
+      cgm.errorNYI("array filler should always be typed");
 
     mlir::Type desiredType = cgm.convertType(destType);
     return emitArrayConstant(cgm, desiredType, commonElementType, numElements,
-                             elements, typedFilter);
+                             elements, typedFiller);
   }
   case APValue::Vector: {
     cgm.errorNYI("ConstExprEmitter::tryEmitPrivate vector");

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

@@ -201,9 +201,9 @@ ConstArrayAttr::verify(function_ref<::mlir::InFlightDiagnostic()> emitError,
   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());
+  if (auto strAttr = mlir::dyn_cast<StringAttr>(elts)) {
+    const auto arrayTy = mlir::cast<ArrayType>(type);
+    const auto intTy = mlir::dyn_cast<IntType>(arrayTy.getEltType());
 
     // TODO: add CIR type for char.
     if (!intTy || intTy.getWidth() != 8) {
@@ -215,8 +215,8 @@ ConstArrayAttr::verify(function_ref<::mlir::InFlightDiagnostic()> emitError,
   }
 
   assert(mlir::isa<ArrayAttr>(elts));
-  ArrayAttr arrayAttr = mlir::cast<mlir::ArrayAttr>(elts);
-  ArrayType arrayTy = mlir::cast<ArrayType>(type);
+  const auto arrayAttr = mlir::cast<mlir::ArrayAttr>(elts);
+  const auto arrayTy = mlir::cast<ArrayType>(type);
 
   // Make sure both number of elements and subelement types match type.
   if (arrayTy.getSize() != arrayAttr.size() + trailingZerosNum)
@@ -225,10 +225,9 @@ ConstArrayAttr::verify(function_ref<::mlir::InFlightDiagnostic()> emitError,
 }
 
 Attribute ConstArrayAttr::parse(AsmParser &parser, Type type) {
-  ::mlir::FailureOr<Type> resultTy;
-  ::mlir::FailureOr<Attribute> resultVal;
-  ::llvm::SMLoc loc = parser.getCurrentLocation();
-  (void)loc;
+  mlir::FailureOr<Type> resultTy;
+  mlir::FailureOr<Attribute> resultVal;
+
   // Parse literal '<'
   if (parser.parseLess())
     return {};
@@ -244,7 +243,7 @@ Attribute ConstArrayAttr::parse(AsmParser &parser, Type type) {
   }
 
   // ArrayAttrrs have per-element type, not the type of the array...
-  if (mlir::dyn_cast<ArrayAttr>(*resultVal)) {
+  if (mlir::isa<ArrayAttr>(*resultVal)) {
     // Array has implicit type: infer from const array type.
     if (parser.parseOptionalColon().failed()) {
       resultTy = type;
@@ -259,7 +258,6 @@ Attribute ConstArrayAttr::parse(AsmParser &parser, Type type) {
       }
     }
   } else {
-    assert(mlir::isa<TypedAttr>(*resultVal) && "IDK");
     auto ta = mlir::cast<TypedAttr>(*resultVal);
     resultTy = ta.getType();
     if (mlir::isa<mlir::NoneType>(*resultTy)) {
@@ -269,11 +267,11 @@ Attribute ConstArrayAttr::parse(AsmParser &parser, Type type) {
     }
   }
 
-  auto zeros = 0;
+  unsigned 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();
+      unsigned typeSize = mlir::cast<cir::ArrayType>(resultTy.value()).getSize();
+      mlir::Attribute elts = resultVal.value();
       if (auto str = mlir::dyn_cast<mlir::StringAttr>(elts))
         zeros = typeSize - str.size();
       else
@@ -288,7 +286,7 @@ Attribute ConstArrayAttr::parse(AsmParser &parser, Type type) {
     return {};
 
   return parser.getChecked<ConstArrayAttr>(
-      loc, parser.getContext(), resultTy.value(), resultVal.value(), zeros);
+      parser.getCurrentLocation(), parser.getContext(), resultTy.value(), resultVal.value(), zeros);
 }
 
 void ConstArrayAttr::print(AsmPrinter &printer) const {

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

@@ -189,7 +189,7 @@ mlir::Value lowerCirAttrAsValue(mlir::Operation *parentOp,
                                 mlir::ConversionPatternRewriter &rewriter,
                                 const mlir::TypeConverter *converter) {
   CIRAttrToValue valueConverter(parentOp, rewriter, converter);
-  auto value = valueConverter.visit(attr);
+  mlir::Value value = valueConverter.visit(attr);
   if (!value)
     llvm_unreachable("unhandled attribute type");
   return value;
@@ -242,8 +242,7 @@ mlir::Value CIRAttrToValue::visitCirAttr(cir::ConstArrayAttr attr) {
   if (auto arrayAttr = mlir::dyn_cast<mlir::ArrayAttr>(attr.getElts())) {
     for (auto [idx, elt] : llvm::enumerate(arrayAttr)) {
       mlir::DataLayout dataLayout(parentOp->getParentOfType<mlir::ModuleOp>());
-      mlir::Value init =
-          emitCirAttrToMemory(parentOp, elt, rewriter, converter, dataLayout);
+      mlir::Value init = visit(elt);
       result =
           rewriter.create<mlir::LLVM::InsertValueOp>(loc, result, init, idx);
     }