Update on "[CIR][CIRGen][Bugfix] Update unions to track all members"

This diverges from the original codegen by tracking all members of a
union, instead of just the largest one. This is necessary to support
type-checking at the MLIR level when accessing union members. It also
preserves more information about the source code, which might be useful.

Fixes #224

[ghstack-poisoned]

Author: sitio-couto

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

@@ -452,4 +452,25 @@ def OptNoneAttr : CIRUnitAttr<"OptNone", "optnone"> {
   let storageType = [{ OptNoneAttr }];
 }
 
+def GlobalCtorAttr : CIR_Attr<"GlobalCtor", "globalCtor"> {
+  let summary = "Indicates a function is a global constructor.";
+  let description = [{
+    Describing a global constructor with an optional priority.
+  }];
+  let parameters = (ins "StringAttr":$name,
+                        OptionalParameter<"std::optional<int>">:$priority);
+  let assemblyFormat = [{
+    `<`
+      $name
+      (`,` $priority^)?
+    `>`
+  }];
+  let builders = [
+    AttrBuilder<(ins "StringRef":$name,
+                      CArg<"std::optional<int>", "{}">:$priority), [{
+      return $_get($_ctxt, StringAttr::get($_ctxt, name), priority);
+    }]>
+  ];
+  let skipDefaultBuilders = 1;
+}
 #endif // MLIR_CIR_DIALECT_CIR_ATTRS

clang/lib/CIR/CodeGen/CIRGenBuilder.h

@@ -13,6 +13,7 @@
 #include "CIRGenTypeCache.h"
 #include "UnimplementedFeatureGuarding.h"
 
+#include "clang/AST/Decl.h"
 #include "clang/AST/Type.h"
 #include "clang/CIR/Dialect/IR/CIRAttrs.h"
 #include "clang/CIR/Dialect/IR/CIRDialect.h"
@@ -190,8 +191,7 @@ class CIRGenBuilderTy : public mlir::OpBuilder {
     }
 
     if (!ty)
-      ty = getAnonStructTy(mlir::cir::StructType::Struct, members,
-                           /*body=*/true, packed);
+      ty = getAnonStructTy(members, /*body=*/true, packed);
 
     auto sTy = ty.dyn_cast<mlir::cir::StructType>();
     assert(sTy && "expected struct type");
@@ -381,10 +381,9 @@ class CIRGenBuilderTy : public mlir::OpBuilder {
 
   /// Get a CIR anonymous struct type.
   mlir::cir::StructType
-  getAnonStructTy(mlir::cir::StructType::RecordKind kind,
-                  llvm::ArrayRef<mlir::Type> members, bool body,
+  getAnonStructTy(llvm::ArrayRef<mlir::Type> members, bool body,
                   bool packed = false, const clang::RecordDecl *ast = nullptr) {
-    return getStructTy(kind, members, "", body, packed, ast);
+    return getStructTy(members, "", body, packed, ast);
   }
 
   /// Get a CIR record kind from a AST declaration tag.
@@ -405,14 +404,16 @@ class CIRGenBuilderTy : public mlir::OpBuilder {
   }
 
   /// Get a CIR named struct type.
-  mlir::cir::StructType getStructTy(mlir::cir::StructType::RecordKind kind,
-                                    llvm::ArrayRef<mlir::Type> members,
+  mlir::cir::StructType getStructTy(llvm::ArrayRef<mlir::Type> members,
                                     llvm::StringRef name, bool body,
                                     bool packed, const clang::RecordDecl *ast) {
     const auto nameAttr = getStringAttr(name);
     std::optional<mlir::cir::ASTRecordDeclAttr> astAttr = std::nullopt;
-    if (ast)
+    auto kind = mlir::cir::StructType::RecordKind::Struct;
+    if (ast) {
       astAttr = getAttr<mlir::cir::ASTRecordDeclAttr>(ast);
+      kind = getRecordKind(ast->getTagKind());
+    }
     return mlir::cir::StructType::get(getContext(), members, nameAttr, body,
                                       packed, kind, astAttr);
   }

clang/lib/CIR/CodeGen/CIRGenExpr.cpp

@@ -1142,34 +1142,37 @@ static mlir::Value buildArrayAccessOp(mlir::OpBuilder &builder,
                                       mlir::Location arrayLocBegin,
                                       mlir::Location arrayLocEnd,
                                       mlir::Value arrayPtr, mlir::Type eltTy,
-                                      mlir::Value idx) {
-  mlir::Value basePtr =
-      maybeBuildArrayDecay(builder, arrayLocBegin, arrayPtr, eltTy);
+                                      mlir::Value idx, bool shouldDecay) {
+  mlir::Value basePtr = arrayPtr;
+  if (shouldDecay)
+    basePtr = maybeBuildArrayDecay(builder, arrayLocBegin, arrayPtr, eltTy);
   mlir::Type flatPtrTy = basePtr.getType();
 
   return builder.create<mlir::cir::PtrStrideOp>(arrayLocEnd, flatPtrTy, basePtr,
                                                 idx);
 }
 
-static mlir::Value buildArraySubscriptPtr(
-    CIRGenFunction &CGF, mlir::Location beginLoc, mlir::Location endLoc,
-    mlir::Value ptr, mlir::Type eltTy, ArrayRef<mlir::Value> indices,
-    bool inbounds, bool signedIndices, const llvm::Twine &name = "arrayidx") {
+static mlir::Value
+buildArraySubscriptPtr(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") {
   assert(indices.size() == 1 && "cannot handle multiple indices yet");
   auto idx = indices.back();
   auto &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(!UnimplementedFeature::emitCheckedInBoundsGEP() && "NYI");
-  return buildArrayAccessOp(CGM.getBuilder(), beginLoc, endLoc, ptr, eltTy,
-                            idx);
+  return buildArrayAccessOp(CGM.getBuilder(), beginLoc, endLoc, ptr, eltTy, idx,
+                            shouldDecay);
 }
 
 static Address buildArraySubscriptPtr(
     CIRGenFunction &CGF, mlir::Location beginLoc, mlir::Location endLoc,
     Address addr, ArrayRef<mlir::Value> indices, QualType eltType,
-    bool inbounds, bool signedIndices, mlir::Location loc,
+    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
@@ -1189,7 +1192,7 @@ static Address buildArraySubscriptPtr(
       (!CGF.IsInPreservedAIRegion && !isPreserveAIArrayBase(CGF, Base))) {
     eltPtr = buildArraySubscriptPtr(CGF, beginLoc, endLoc, addr.getPointer(),
                                     addr.getElementType(), indices, inbounds,
-                                    signedIndices, name);
+                                    signedIndices, shouldDecay, name);
   } else {
     // assert(!UnimplementedFeature::generateDebugInfo() && "NYI");
     // assert(indices.size() == 1 && "cannot handle multiple indices yet");
@@ -1279,7 +1282,8 @@ LValue CIRGenFunction::buildArraySubscriptExpr(const ArraySubscriptExpr *E,
         *this, CGM.getLoc(Array->getBeginLoc()), CGM.getLoc(Array->getEndLoc()),
         ArrayLV.getAddress(), {Idx}, E->getType(),
         !getLangOpts().isSignedOverflowDefined(), SignedIndices,
-        CGM.getLoc(E->getExprLoc()), &arrayType, E->getBase());
+        CGM.getLoc(E->getExprLoc()), /*shouldDecay=*/true, &arrayType,
+        E->getBase());
     EltBaseInfo = ArrayLV.getBaseInfo();
     // TODO(cir): EltTBAAInfo
     assert(!UnimplementedFeature::tbaa() && "TBAA is NYI");
@@ -1293,7 +1297,8 @@ LValue CIRGenFunction::buildArraySubscriptExpr(const ArraySubscriptExpr *E,
     Addr = buildArraySubscriptPtr(
         *this, CGM.getLoc(E->getBeginLoc()), CGM.getLoc(E->getEndLoc()), Addr,
         Idx, E->getType(), !getLangOpts().isSignedOverflowDefined(),
-        SignedIndices, CGM.getLoc(E->getExprLoc()), &ptrType, E->getBase());
+        SignedIndices, CGM.getLoc(E->getExprLoc()), /*shouldDecay=*/false,
+        &ptrType, E->getBase());
   }
 
   LValue LV = LValue::makeAddr(Addr, E->getType(), EltBaseInfo);

clang/lib/CIR/CodeGen/CIRGenTypes.cpp

@@ -166,10 +166,8 @@ mlir::Type CIRGenTypes::convertRecordDeclType(const clang::RecordDecl *RD) {
 
   // Handle forward decl / incomplete types.
   if (!entry) {
-    auto recordKind = Builder.getRecordKind(RD->getTagKind());
     auto name = getRecordTypeName(RD, "");
-    entry = Builder.getStructTy(recordKind, {}, name, /*body=*/false,
-                                /*packed=*/false, RD);
+    entry = Builder.getStructTy({}, name, /*body=*/false, /*packed=*/false, RD);
     recordDeclTypes[key] = entry;
   }
 

clang/lib/CIR/CodeGen/CIRGenVTables.cpp

@@ -59,8 +59,7 @@ mlir::Type CIRGenVTables::getVTableType(const VTableLayout &layout) {
 
   // FIXME(cir): should VTableLayout be encoded like we do for some
   // AST nodes?
-  return CGM.getBuilder().getAnonStructTy(mlir::cir::StructType::Struct, tys,
-                                          /*body=*/true);
+  return CGM.getBuilder().getAnonStructTy(tys, /*body=*/true);
 }
 
 /// At this point in the translation unit, does it appear that can we

clang/lib/CIR/CodeGen/CIRRecordLayoutBuilder.cpp

@@ -584,9 +584,7 @@ CIRGenTypes::computeRecordLayout(const RecordDecl *D,
         builder.astRecordLayout.getSize()) {
       CIRRecordLowering baseBuilder(*this, D, /*Packed=*/builder.isPacked);
       auto baseIdentifier = getRecordTypeName(D, ".base");
-      auto recordKind = Builder.getRecordKind(D->getTagKind());
-      *BaseTy = Builder.getStructTy(recordKind, baseBuilder.fieldTypes,
-                                    baseIdentifier,
+      *BaseTy = Builder.getStructTy(baseBuilder.fieldTypes, baseIdentifier,
                                     /*body=*/true, /*packed=*/false, D);
       // TODO(cir): add something like addRecordTypeName
 
@@ -600,8 +598,7 @@ CIRGenTypes::computeRecordLayout(const RecordDecl *D,
   // Fill in the struct *after* computing the base type.  Filling in the body
   // signifies that the type is no longer opaque and record layout is complete,
   // but we may need to recursively layout D while laying D out as a base type.
-  *Ty = Builder.getStructTy(Builder.getRecordKind(D->getTagKind()),
-                            builder.fieldTypes, getRecordTypeName(D, ""),
+  *Ty = Builder.getStructTy(builder.fieldTypes, getRecordTypeName(D, ""),
                             /*body=*/true, /*packed=*/false, D);
 
   auto RL = std::make_unique<CIRGenRecordLayout>(

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

@@ -2237,9 +2237,9 @@ LogicalResult GetMemberOp::verify() {
   if (recordTy.getMembers().size() <= getIndex())
     return emitError() << "member index out of bounds";
 
-  // FIXME(cir): Member type check is disabled for classes since there is a bug
-  // in codegen index for classes.
-  if (!recordTy.isClass() &&
+  // FIXME(cir): Member type check is disabled for classes and incomplete types
+  // as the codegen for these still need to be patched.
+  if (!recordTy.isClass() && !recordTy.getBody() &&
       recordTy.getMembers()[getIndex()] != getResultTy().getPointee())
     return emitError() << "member type mismatch";
 

clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp

@@ -8,6 +8,7 @@
 
 #include "PassDetail.h"
 #include "mlir/Dialect/Func/IR/FuncOps.h"
+#include "mlir/IR/BuiltinAttributes.h"
 #include "mlir/IR/Region.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Mangle.h"
@@ -125,6 +126,8 @@ void LoweringPreparePass::lowerGlobalOp(GlobalOp op) {
     ctorRegion.getBlocks().clear();
 
     // Add a function call to the variable initialization function.
+    assert(!op.getAst()->getAstDecl()->getAttr<clang::InitPriorityAttr>() &&
+           "custom initialization priority NYI");
     dynamicInitializers.push_back(f);
   }
 }
@@ -133,6 +136,17 @@ void LoweringPreparePass::buildCXXGlobalInitFunc() {
   if (dynamicInitializers.empty())
     return;
 
+  SmallVector<mlir::Attribute, 4> attrs;
+  for (auto &f : dynamicInitializers) {
+    // TODO: handle globals with a user-specified initialzation priority.
+    auto ctorAttr =
+        mlir::cir::GlobalCtorAttr::get(&getContext(), f.getName());
+    attrs.push_back(ctorAttr);
+  }
+
+  theModule->setAttr("cir.globalCtors",
+                     mlir::ArrayAttr::get(&getContext(), attrs));
+
   SmallString<256> fnName;
   // Include the filename in the symbol name. Including "sub_" matches gcc
   // and makes sure these symbols appear lexicographically behind the symbols
@@ -161,9 +175,9 @@ void LoweringPreparePass::buildCXXGlobalInitFunc() {
       builder.getContext(), mlir::cir::GlobalLinkageKind::ExternalLinkage));
   mlir::SymbolTable::setSymbolVisibility(
       f, mlir::SymbolTable::Visibility::Private);
-  mlir::NamedAttrList attrs;
+  mlir::NamedAttrList extraAttrs;
   f.setExtraAttrsAttr(mlir::cir::ExtraFuncAttributesAttr::get(
-      builder.getContext(), attrs.getDictionary(builder.getContext())));
+      builder.getContext(), extraAttrs.getDictionary(builder.getContext())));
 
   builder.setInsertionPointToStart(f.addEntryBlock());
   for (auto &f : dynamicInitializers) {

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

@@ -57,6 +57,7 @@
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/Sequence.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/Support/Casting.h"
@@ -1829,6 +1830,79 @@ mlir::LLVMTypeConverter prepareTypeConverter(mlir::MLIRContext *ctx) {
 }
 } // namespace
 
+static void buildCtorList(mlir::ModuleOp module) {
+  llvm::SmallVector<std::pair<StringRef, int>, 2> globalCtors;
+  for (auto namedAttr : module->getAttrs()) {
+    if (namedAttr.getName() == "cir.globalCtors") {
+      for (auto attr : namedAttr.getValue().cast<mlir::ArrayAttr>()) {
+        assert(attr.isa<mlir::cir::GlobalCtorAttr>() &&
+               "must be a GlobalCtorAttr");
+        if (auto ctorAttr = attr.cast<mlir::cir::GlobalCtorAttr>()) {
+           // default priority is 65536
+          int priority = 65536;
+          if (ctorAttr.getPriority())
+            priority = *ctorAttr.getPriority();
+          globalCtors.emplace_back(ctorAttr.getName(), priority);
+        }
+      }
+      break;
+    }
+  }
+
+  if (globalCtors.empty())
+    return;
+
+  mlir::OpBuilder builder(module.getContext());
+  builder.setInsertionPointToEnd(&module.getBodyRegion().back());
+
+  // Create a global array llvm.global_ctors with element type of
+  // struct { i32, ptr, ptr }
+  auto CtorPFTy = mlir::LLVM::LLVMPointerType::get(builder.getContext());
+  llvm::SmallVector<mlir::Type> CtorStructFields;
+  CtorStructFields.push_back(builder.getI32Type());
+  CtorStructFields.push_back(CtorPFTy);
+  CtorStructFields.push_back(CtorPFTy);
+
+  auto CtorStructTy = mlir::LLVM::LLVMStructType::getLiteral(
+      builder.getContext(), CtorStructFields);
+  auto CtorStructArrayTy =
+      mlir::LLVM::LLVMArrayType::get(CtorStructTy, globalCtors.size());
+
+  auto loc = module.getLoc();
+  auto newGlobalOp = builder.create<mlir::LLVM::GlobalOp>(
+      loc, CtorStructArrayTy, true, mlir::LLVM::Linkage::Appending,
+      "llvm.global_ctors", mlir::Attribute());
+
+  newGlobalOp.getRegion().push_back(new mlir::Block());
+  builder.setInsertionPointToEnd(newGlobalOp.getInitializerBlock());
+
+  mlir::Value result = builder.create<mlir::LLVM::UndefOp>(
+      loc, CtorStructArrayTy);
+
+  for (uint64_t I = 0; I < globalCtors.size(); I++) {
+    auto fn = globalCtors[I];
+    mlir::Value structInit =
+        builder.create<mlir::LLVM::UndefOp>(loc, CtorStructTy);
+    mlir::Value initPriority =
+        builder.create<mlir::LLVM::ConstantOp>(loc, CtorStructFields[0], fn.second);
+    mlir::Value initFuncAddr = builder.create<mlir::LLVM::AddressOfOp>(
+        loc, CtorStructFields[1], fn.first);
+    mlir::Value initAssociate =
+        builder.create<mlir::LLVM::NullOp>(loc, CtorStructFields[2]);
+    structInit = builder.create<mlir::LLVM::InsertValueOp>(loc, structInit,
+                                                           initPriority, 0);
+    structInit = builder.create<mlir::LLVM::InsertValueOp>(loc, structInit,
+                                                           initFuncAddr, 1);
+    // TODO: handle associated data for initializers.
+    structInit = builder.create<mlir::LLVM::InsertValueOp>(loc, structInit,
+                                                           initAssociate, 2);
+    result =
+        builder.create<mlir::LLVM::InsertValueOp>(loc, result, structInit, I);
+  }
+
+  builder.create<mlir::LLVM::ReturnOp>(loc, result);
+}
+
 void ConvertCIRToLLVMPass::runOnOperation() {
   auto module = getOperation();
 
@@ -1870,6 +1944,9 @@ void ConvertCIRToLLVMPass::runOnOperation() {
 
   if (failed(applyPartialConversion(module, target, std::move(patterns))))
     signalPassFailure();
+
+  // Emit the llvm.global_ctors array.
+  buildCtorList(module);
 }
 
 std::unique_ptr<mlir::Pass> createConvertCIRToLLVMPass() {

clang/test/CIR/CodeGen/array.cpp

@@ -71,3 +71,22 @@ struct S {
   int i;
 } arr[3] = {{1}};
 // CHECK: cir.global external @arr = #cir.const_array<[#cir.const_struct<{#cir.int<1> : !s32i}> : !ty_22S22, #cir.zero : !ty_22S22, #cir.zero : !ty_22S22]> : !cir.array<!ty_22S22 x 3>
+
+void testPointerDecaySubscriptAccess(int arr[]) {
+// CHECK: cir.func @{{.+}}testPointerDecaySubscriptAccess
+  arr[1];
+  // CHECK: %[[#BASE:]] = cir.load %{{.+}} : cir.ptr <!cir.ptr<!s32i>>, !cir.ptr<!s32i>
+  // CHECK: %[[#DIM1:]] = cir.const(#cir.int<1> : !s32i) : !s32i
+  // CHECK: cir.ptr_stride(%[[#BASE]] : !cir.ptr<!s32i>, %[[#DIM1]] : !s32i), !cir.ptr<!s32i>
+}
+
+void testPointerDecayedArrayMultiDimSubscriptAccess(int arr[][3]) {
+// CHECK: cir.func @{{.+}}testPointerDecayedArrayMultiDimSubscriptAccess
+  arr[1][2];
+  // CHECK: %[[#V1:]] = cir.load %{{.+}} : cir.ptr <!cir.ptr<!cir.array<!s32i x 3>>>, !cir.ptr<!cir.array<!s32i x 3>>
+  // CHECK: %[[#V2:]] = cir.const(#cir.int<1> : !s32i) : !s32i
+  // CHECK: %[[#V3:]] = cir.ptr_stride(%[[#V1]] : !cir.ptr<!cir.array<!s32i x 3>>, %[[#V2]] : !s32i), !cir.ptr<!cir.array<!s32i x 3>>
+  // CHECK: %[[#V4:]] = cir.const(#cir.int<2> : !s32i) : !s32i
+  // CHECK: %[[#V5:]] = cir.cast(array_to_ptrdecay, %[[#V3]] : !cir.ptr<!cir.array<!s32i x 3>>), !cir.ptr<!s32i>
+  // CHECK: cir.ptr_stride(%[[#V5]] : !cir.ptr<!s32i>, %[[#V4]] : !s32i), !cir.ptr<!s32i>
+}