[CIR] Introduce LocalInitOp, & lower static locals

[erichkeane]

During an investigation of something else, I discovered that our handling of static-local as a ctor/dtor on a GlobalOp meant that it couldn't actually be initialized with reference to any local, parameter, or member declarations. This is obviously problematic.

This patch instead introduces a LocalInitOp, which is an operation that represents the location of initialization for the static local. This is lowered during lowering-prepare, same as we did previously (in fact, it uses basically the exact same lowering code, with some slight modifications).

Lowering from AST itself splits slightly from global declarations, but the two share implementation as closely as possible.

At the moment, this operation only works for static-locals, and has a handful of asserts to do the same. It is intended that the thread-local-storage use the exact same mechanism, with some slight modifications to the lowering-prepare pass to introduce the different init behavior.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangir

Author: Erich Keane (erichkeane)

Changes

During an investigation of something else, I discovered that our handling of static-local as a ctor/dtor on a GlobalOp meant that it couldn't actually be initialized with reference to any local, parameter, or member declarations. This is obviously problematic.

This patch instead introduces a LocalInitOp, which is an operation that represents the location of initialization for the static local. This is lowered during lowering-prepare, same as we did previously (in fact, it uses basically the exact same lowering code, with some slight modifications).

Lowering from AST itself splits slightly from global declarations, but the two share implementation as closely as possible.

At the moment, this operation only works for static-locals, and has a handful of asserts to do the same. It is intended that the thread-local-storage use the exact same mechanism, with some slight modifications to the lowering-prepare pass to introduce the different init behavior.

---

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

11 Files Affected:

• (modified) clang/include/clang/CIR/Dialect/IR/CIROps.td (+46-1)
• (modified) clang/lib/CIR/CodeGen/CIRGenCXX.cpp (+53-21)
• (modified) clang/lib/CIR/CodeGen/CIRGenDecl.cpp (+2-1)
• (modified) clang/lib/CIR/CodeGen/CIRGenDeclCXX.cpp (+22-1)
• (modified) clang/lib/CIR/CodeGen/CIRGenFunction.h (+1-2)
• (modified) clang/lib/CIR/CodeGen/CIRGenModule.h (+8)
• (modified) clang/lib/CIR/Dialect/IR/CIRDialect.cpp (+46)
• (modified) clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp (+121-91)
• (modified) clang/test/CIR/CodeGen/static-local.cpp (+538-54)
• (modified) clang/test/CIR/IR/invalid-static-local.cir (+65)
• (modified) clang/test/CIR/IR/static-local.cir (+42)

diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td
index f20ba262d6480..f21683b1de82e 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIROps.td
+++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td
@@ -956,7 +956,7 @@ def CIR_ConditionOp : CIR_Op<"condition", [
 defvar CIR_YieldableScopes = [
   "ArrayCtor", "ArrayDtor", "AwaitOp", "CaseOp", "CleanupScopeOp", "DoWhileOp",
   "ForOp", "GlobalOp", "IfOp", "ScopeOp", "SwitchOp", "TernaryOp", "TryOp",
-  "WhileOp"
+  "WhileOp", "LocalInitOp"
 ];
 
 def CIR_YieldOp : CIR_Op<"yield", [
@@ -2975,6 +2975,51 @@ def CIR_GetGlobalOp : CIR_Op<"get_global", [
   }];
 }
 
+//===----------------------------------------------------------------------===//
+// LocalInitOp
+//===----------------------------------------------------------------------===//
+
+def CIR_LocalInitOp : CIR_Op<"local_init", [
+  DeclareOpInterfaceMethods<SymbolUserOpInterface>, NoRegionArguments
+]> {
+  let summary = "initialize a static or thread local object";
+  let description = [{
+    The 'cir.local_init' operation has no result, but is responsible for
+    containing the regions to initialize and destroy the static local
+    variable. This will be handled during lowering-prepare to include the
+    guard variables correctly for the variable.
+
+    Example:
+    ```
+    cir.local_init thread_local @GlobalName ctor {
+      %4 = cir.get_global static_local @GlobalName : !cir.ptr<!rec_CtorDtor>
+      %5 = cir.call @_Z5get_iv() : () -> !s32i
+      cir.call @_ZN8CtorDtorC1Ei(%4, %5) : !cir.ptr<!rec_CtorDtor>
+      cir.yield
+    }, dtor {
+      %4 = cir.get_global static_local @_ZZ3foovE8localCD2 :
+                                                  !cir.ptr<!rec_CtorDtor>
+      cir.call @_ZN8CtorDtorD1Ev(%4) : (!cir.ptr<!rec_CtorDtor>) -> ()
+      cir.yield
+    }
+  }];
+  let arguments = (ins FlatSymbolRefAttr:$globalName, UnitAttr:$tls,
+                       UnitAttr:$static_local);
+  let regions = (region MaxSizedRegion<1>:$ctorRegion,
+                        MaxSizedRegion<1>:$dtorRegion);
+
+  let assemblyFormat = [{
+    (`thread_local` $tls^)?
+    (`static_local` $static_local^)?
+    $globalName attr-dict
+    (`ctor` $ctorRegion^)?
+    (`dtor` $dtorRegion^)?
+  }];
+
+  let hasLLVMLowering = false;
+  let hasVerifier = 1;
+}
+
 //===----------------------------------------------------------------------===//
 // VTableAddrPointOp
 //===----------------------------------------------------------------------===//
diff --git a/clang/lib/CIR/CodeGen/CIRGenCXX.cpp b/clang/lib/CIR/CodeGen/CIRGenCXX.cpp
index c111705783773..7ce8fb1f5903f 100644
--- a/clang/lib/CIR/CodeGen/CIRGenCXX.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenCXX.cpp
@@ -52,7 +52,7 @@ void CIRGenFunction::emitInvariantStart(CharUnits size, mlir::Value addr,
 }
 
 static void emitDeclInit(CIRGenFunction &cgf, const VarDecl *varDecl,
-                         cir::GlobalOp globalOp) {
+                         cir::GlobalOp globalOp, mlir::Region &ctorRegion) {
   assert((varDecl->hasGlobalStorage() ||
           (varDecl->hasLocalStorage() &&
            cgf.getContext().getLangOpts().OpenCLCPlusPlus)) &&
@@ -64,7 +64,7 @@ static void emitDeclInit(CIRGenFunction &cgf, const VarDecl *varDecl,
 
   // Set up the ctor region.
   mlir::OpBuilder::InsertionGuard guard(builder);
-  mlir::Block *block = builder.createBlock(&globalOp.getCtorRegion());
+  mlir::Block *block = builder.createBlock(&ctorRegion);
   CIRGenFunction::LexicalScope lexScope{cgf, globalOp.getLoc(),
                                         builder.getInsertionBlock()};
   lexScope.setAsGlobalInit();
@@ -100,7 +100,7 @@ static void emitDeclInit(CIRGenFunction &cgf, const VarDecl *varDecl,
 }
 
 static void emitDeclDestroy(CIRGenFunction &cgf, const VarDecl *vd,
-                            cir::GlobalOp addr) {
+                            cir::GlobalOp addr, mlir::Region &dtorRegion) {
   // Honor __attribute__((no_destroy)) and bail instead of attempting
   // to emit a reference to a possibly nonexistent destructor, which
   // in turn can cause a crash. This will result in a global constructor
@@ -131,7 +131,7 @@ static void emitDeclDestroy(CIRGenFunction &cgf, const VarDecl *vd,
 
   // Prepare the dtor region.
   mlir::OpBuilder::InsertionGuard guard(builder);
-  mlir::Block *block = builder.createBlock(&addr.getDtorRegion());
+  mlir::Block *block = builder.createBlock(&dtorRegion);
   CIRGenFunction::LexicalScope lexScope{cgf, addr.getLoc(),
                                         builder.getInsertionBlock()};
   lexScope.setAsGlobalInit();
@@ -225,10 +225,14 @@ cir::FuncOp CIRGenModule::codegenCXXStructor(GlobalDecl gd) {
 // region to the global variable and insert the initialization code
 // into the ctor region. This will be moved into the
 // __cxx_global_var_init function during the LoweringPrepare pass.
-void CIRGenModule::emitCXXGlobalVarDeclInit(const VarDecl *varDecl,
-                                            cir::GlobalOp addr,
-                                            bool performInit) {
+void CIRGenModule::emitCXXSpecialVarDeclInit(const VarDecl *varDecl,
+                                             cir::GlobalOp addr,
+                                             bool performInit,
+                                             mlir::Region &ctorRegion,
+                                             mlir::Region &dtorRegion) {
   QualType ty = varDecl->getType();
+  assert(curCGF && "Special var init only available inside of a function");
+  CIRGenFunction &cgf = *curCGF;
 
   // TODO: handle address space
   // The address space of a static local variable (addr) may be different
@@ -248,15 +252,6 @@ void CIRGenModule::emitCXXGlobalVarDeclInit(const VarDecl *varDecl,
   // expects "this" in the "generic" address space.
   assert(!cir::MissingFeatures::addressSpace());
 
-  // Create a CIRGenFunction to emit the initializer. While this isn't a true
-  // function, the handling works the same way.
-  CIRGenFunction cgf{*this, builder, true};
-  llvm::SaveAndRestore<CIRGenFunction *> savedCGF(curCGF, &cgf);
-  curCGF->curFn = addr;
-
-  CIRGenFunction::SourceLocRAIIObject fnLoc{cgf,
-                                            getLoc(varDecl->getLocation())};
-
   addr.setAstAttr(cir::ASTVarDeclAttr::get(&getMLIRContext(), varDecl));
 
   if (!ty->isReferenceType()) {
@@ -268,15 +263,15 @@ void CIRGenModule::emitCXXGlobalVarDeclInit(const VarDecl *varDecl,
         varDecl->getType().isConstantStorage(getASTContext(), true, !needsDtor);
     // PerformInit, constant store invariant / destroy handled below.
     if (performInit) {
-      emitDeclInit(cgf, varDecl, addr);
+      emitDeclInit(cgf, varDecl, addr, ctorRegion);
       // For constant storage, emit invariant.start in the ctor region after
       // initialization but before the yield.
       if (isConstantStorage) {
         CIRGenBuilderTy &builder = cgf.getBuilder();
         mlir::OpBuilder::InsertionGuard guard(builder);
         // Set insertion point to end of ctor region (before yield)
-        if (!addr.getCtorRegion().empty()) {
-          mlir::Block *block = &addr.getCtorRegion().back();
+        if (!ctorRegion.empty()) {
+          mlir::Block *block = &ctorRegion.back();
           // Find the yield op and insert before it
           mlir::Operation *yieldOp = block->getTerminator();
           if (yieldOp) {
@@ -290,12 +285,12 @@ void CIRGenModule::emitCXXGlobalVarDeclInit(const VarDecl *varDecl,
     }
 
     if (!isConstantStorage)
-      emitDeclDestroy(cgf, varDecl, addr);
+      emitDeclDestroy(cgf, varDecl, addr, dtorRegion);
     return;
   }
 
   mlir::OpBuilder::InsertionGuard guard(builder);
-  auto *block = builder.createBlock(&addr.getCtorRegion());
+  auto *block = builder.createBlock(&ctorRegion);
   CIRGenFunction::LexicalScope scope{*curCGF, addr.getLoc(),
                                      builder.getInsertionBlock()};
   scope.setAsGlobalInit();
@@ -325,3 +320,40 @@ void CIRGenModule::emitCXXGlobalVarDeclInit(const VarDecl *varDecl,
   builder.setInsertionPointToEnd(block);
   cir::YieldOp::create(builder, addr->getLoc());
 }
+
+void CIRGenModule::emitCXXGlobalVarDeclInit(const VarDecl *varDecl,
+                                            cir::GlobalOp addr,
+                                            bool performInit) {
+  assert(!varDecl->isStaticLocal() &&
+         varDecl->getTLSKind() == VarDecl::TLS_None);
+
+  // Create a CIRGenFunction to emit the initializer. While this isn't a true
+  // function, the handling works the same way.
+  CIRGenFunction cgf{*this, builder, true};
+  llvm::SaveAndRestore<CIRGenFunction *> savedCGF(curCGF, &cgf);
+  curCGF->curFn = addr;
+
+  CIRGenFunction::SourceLocRAIIObject fnLoc{cgf,
+                                            getLoc(varDecl->getLocation())};
+
+  emitCXXSpecialVarDeclInit(varDecl, addr, performInit, addr.getCtorRegion(),
+                            addr.getDtorRegion());
+}
+
+void CIRGenModule::emitCXXStaticLocalVarDeclInit(const VarDecl *varDecl,
+                                                 cir::GlobalOp addr,
+                                                 bool performInit) {
+  assert(varDecl->isStaticLocal() ||
+         varDecl->getTLSKind() != VarDecl::TLS_None);
+
+  if (varDecl->getTLSKind() != VarDecl::TLS_None)
+    errorNYI(varDecl->getSourceRange(),
+             "TLS not implemented for static-local init");
+
+  auto initOp = cir::LocalInitOp::create(
+      builder, addr->getLoc(), addr.getSymNameAttr(),
+      varDecl->getTLSKind() != VarDecl::TLS_None, varDecl->isStaticLocal());
+
+  emitCXXSpecialVarDeclInit(varDecl, addr, performInit, initOp.getCtorRegion(),
+                            initOp.getDtorRegion());
+}
diff --git a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
index fce81a458e937..e829260d60aa2 100644
--- a/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenDecl.cpp
@@ -627,7 +627,8 @@ cir::GlobalOp CIRGenFunction::addInitializerToStaticVarDecl(
     // We have a constant initializer, but a nontrivial destructor. We still
     // need to perform a guarded "initialization" in order to register the
     // destructor.
-    cgm.errorNYI(d.getSourceRange(), "C++ guarded init");
+    emitCXXGuardedInit(d, gv, /*performInit=*/true);
+    gvAddr.setStaticLocal(true);
   }
 
   return gv;
diff --git a/clang/lib/CIR/CodeGen/CIRGenDeclCXX.cpp b/clang/lib/CIR/CodeGen/CIRGenDeclCXX.cpp
index 255e5ead77072..955f3f4815a06 100644
--- a/clang/lib/CIR/CodeGen/CIRGenDeclCXX.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenDeclCXX.cpp
@@ -43,11 +43,20 @@ void CIRGenFunction::emitCXXGuardedInit(const VarDecl &varDecl,
   // Mark the global as static local with the guard name. The emission of the
   // guard/acquire is done during LoweringPrepare.
   auto guardAttr = mlir::StringAttr::get(&cgm.getMLIRContext(), guardName);
+  if (!varDecl.isStaticLocal())
+    cgm.errorNYI(
+        varDecl.getSourceRange(),
+        "Static local guard attr only valid on static local variables");
   globalOp.setStaticLocalGuardAttr(
       cir::StaticLocalGuardAttr::get(&cgm.getMLIRContext(), guardAttr));
 
   // Emit the initializer and add a global destructor if appropriate.
-  cgm.emitCXXGlobalVarDeclInit(&varDecl, globalOp, performInit);
+  // TODO(cir): classic codegen calls emitCXXGlobalVarDeclInit for this as well,
+  // and this is meant to handle cases with weak linkage (see comment in
+  // emitCXXGlobalVarDeclInitFunc). At one point we'll have to do some level of
+  // split here depending on whether this is a global (which should/can have
+  // ctor/dtor regions), or should have in-function initialization.
+  cgm.emitCXXStaticLocalVarDeclInit(&varDecl, globalOp, performInit);
 }
 
 void CIRGenModule::emitCXXGlobalVarDeclInitFunc(const VarDecl *vd,
@@ -57,5 +66,17 @@ void CIRGenModule::emitCXXGlobalVarDeclInitFunc(const VarDecl *vd,
 
   assert(!cir::MissingFeatures::deferredCXXGlobalInit());
 
+  // TODO(cir): Classic codegen calls emitCXXGuardedInit in the following case:
+  // template<typename T> struct Templ {
+  //   static T f;
+  // };
+  // template<typename T> T Templ<T>::f = get_i();
+  // auto func() {
+  //   Templ<int> t;
+  //   return decltype(t)::f;
+  // }
+  //
+  // However, at the moment it is only suitable for static-local variables, so
+  // we will have to modify it to work for this case as well.
   emitCXXGlobalVarDeclInit(vd, addr, performInit);
 }
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index c798833d877ea..8da4855a89800 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -2048,8 +2048,7 @@ class CIRGenFunction : public CIRGenTypeCache {
 
   void emitStaticVarDecl(const VarDecl &d, cir::GlobalLinkageKind linkage);
 
-  /// Emit a guarded initializer for a static local variable or a static
-  /// data member of a class template instantiation.
+  /// Emit a guarded initializer for a static local variable.
   void emitCXXGuardedInit(const VarDecl &varDecl, cir::GlobalOp globalOp,
                           bool performInit);
 
diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.h b/clang/lib/CIR/CodeGen/CIRGenModule.h
index 1b999abd4caea..ec93149c32dfd 100644
--- a/clang/lib/CIR/CodeGen/CIRGenModule.h
+++ b/clang/lib/CIR/CodeGen/CIRGenModule.h
@@ -577,6 +577,14 @@ class CIRGenModule : public CIRGenTypeCache {
   void emitGlobalVarDefinition(const clang::VarDecl *vd,
                                bool isTentative = false);
 
+  /// Helper function for the below two that will create the
+  /// constructor/destructor in specified regions, rather than in the GlobalOp.
+  void emitCXXSpecialVarDeclInit(const VarDecl *varDecl, cir::GlobalOp addr,
+                                 bool performInit, mlir::Region &ctorRegion,
+                                 mlir::Region &dtorRegion);
+  /// Emit the function that initializes the specified static-local variable.
+  void emitCXXStaticLocalVarDeclInit(const VarDecl *varDecl, cir::GlobalOp addr,
+                                     bool performInit);
   /// Emit the function that initializes the specified global
   void emitCXXGlobalVarDeclInit(const VarDecl *varDecl, cir::GlobalOp addr,
                                 bool performInit);
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 35a31b0dbda63..0d0ae1a4670a6 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -371,6 +371,45 @@ LogicalResult cir::BreakOp::verify() {
   return success();
 }
 
+//===----------------------------------------------------------------------===//
+// LocalInitOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult cir::LocalInitOp::verify() {
+  if (!getOperation()->getParentOfType<FuncOp>())
+    return emitOpError("must be inside of a function");
+
+  if (getStaticLocal() && getTls())
+    return emitOpError("cannot be both static and thread local");
+
+  if (!getStaticLocal() && !getTls())
+    return emitOpError("must be one of static and thread local");
+
+  return success();
+}
+
+LogicalResult
+cir::LocalInitOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
+  mlir::Operation *op =
+      symbolTable.lookupNearestSymbolFrom(*this, getGlobalNameAttr());
+  if (op == nullptr || !isa<GlobalOp>(op))
+    return emitOpError("'")
+           << getGlobalName() << "' does not reference a valid cir.global";
+
+  auto global = cast<GlobalOp>(op);
+
+  if (getTls() && !global.getTlsModel())
+    return emitOpError("access to global not marked thread local");
+
+  bool isStaticLocal = getStaticLocal();
+  bool globalIsStaticLocal = global.getStaticLocalGuard().has_value();
+
+  if (isStaticLocal != globalIsStaticLocal)
+    return emitOpError("static_local attribute mismatch");
+
+  return success();
+}
+
 //===----------------------------------------------------------------------===//
 // ConditionOp
 //===----------------------------------------------------------------------===//
@@ -1844,6 +1883,13 @@ mlir::LogicalResult cir::GlobalOp::verify() {
       return failure();
   }
 
+  if ((getStaticLocalGuard().has_value() || getTlsModel()) &&
+      (!getCtorRegion().empty() || !getDtorRegion().empty()))
+    return emitOpError(
+        "Cannot have a thread-local or static-local global-op "
+        "with a constructor or destructor, they require in-function "
+        "initialization via LocalInitOp");
+
   // TODO(CIR): Many other checks for properties that haven't been upstreamed
   // yet.
 
diff --git a/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp b/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp
index ec032a92591d7..26694a80d7831 100644
--- a/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp
+++ b/clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp
@@ -94,6 +94,7 @@ struct LoweringPreparePass
   void lowerArrayCtor(cir::ArrayCtor op);
   void lowerTrivialCopyCall(cir::CallOp op);
   void lowerStoreOfConstAggregate(cir::StoreOp op);
+  void lowerLocalInitOp(cir::LocalInitOp op);
 
   /// Build the function that initializes the specified global
   cir::FuncOp buildCXXGlobalVarDeclInitFunc(cir::GlobalOp op);
@@ -135,7 +136,7 @@ struct LoweringPreparePass
                                         FuncOp regGlobalFunc);
 
   /// Handle static local variable initialization with guard variables.
-  void handleStaticLocal(cir::GlobalOp globalOp, cir::GetGlobalOp getGlobalOp);
+  void handleStaticLocal(cir::GlobalOp globalOp, cir::LocalInitOp localInitOp);
 
   /// Get or create __cxa_guard_acquire function.
   cir::FuncOp getGuardAcquireFn(cir::PointerType guardPtrTy);
@@ -238,10 +239,64 @@ struct LoweringPreparePass
     }
   }
 
+  void emitGlobalGuardedDtorRegion(CIRBaseBuilderTy &builder,
+                                   cir::GlobalOp global,
+                                   mlir::Region &dtorRegion,
+                                   mlir::Block &entryBB) {
+    // Create a variable that binds the atexit to this shared object.
+    builder.setInsertionPointToStart(&mlirModule.getBodyRegion().front());
+    cir::GlobalOp handle = buildRuntimeVariable(
+        builder, "__dso_handle", global.getLoc(), builder.getI8Type(),
+        cir::GlobalLinkageKind::ExternalLinkage, cir::VisibilityKind::Hidden);
+
+    // If this is a simple call to a destructor, get the called function.
+    // Otherwise, create a helper function for the entire dtor region,
+    // replacing the current dtor region body with a call to the helper
+    // function.
+    cir::CallOp dtorCall;
+    cir::FuncOp dtorFunc =
+        getOrCreateDtorFunc(builder, global, dtorRegion, dtorCall);
+
+    // Create a runtime helper function:
+    //    extern "C" int __cxa_atexit(void (*f)(void *), void *p, void *d);
+    cir::VoidType voidTy = builder.getVoidTy();
+    auto voidPtrTy = cir::PointerType::get(voidTy);
+    auto voidFnTy = cir::FuncType::get({voidPtrTy}, voidTy);
+    auto voidFnPtrTy = cir::PointerType::get(voidFnTy);
+    auto handlePtrTy = cir::PointerType::get(handle.getSymType());
+    auto fnAtExitType =
+        cir::FuncType::get({voidFnPtrTy, voidPtrTy, handlePtrTy}, voidTy);
+    llvm::StringLiteral nameAtExit = "__cxa_atexit";
+    cir::FuncOp fnAtExit = buildRuntimeFunction(builder, nameAtExit,
+                                                global.getLoc(), fnAtExitType);
+
+    // Replace the dtor (or helper) call with a call to
+    //   __cxa_atexit(&dtor, &var, &__dso_handle)
+    builder.setInsertionPointAfter(dtorCall);
+    mlir::Value args[3];
+    auto dtorPtrTy = cir::PointerType::get(dtorFunc.getFunctionType());
+    args[0] = cir::GetGlobalOp::create(builder, dtorCall.getLoc(), dtorPtrTy,
+                                       dtorFunc.getSymName());
+    args[0] = cir::CastOp::create(builder, dtorCall.getLoc(), voidFnPtrTy,
+                                  cir::CastKind::bitcast, args[0]);
+    args[1] =
+        cir::CastOp::create(builder, dtorCall.getLoc(), voidPtrTy,
+                            cir::CastKind::bitcast, dtorCall.getArgOperand(0));
+    args[2] = cir::GetGlobalOp::create(builder, handle.getLoc(), handlePtrTy,
+                                       handle.getSymName());
+    builder.createCallOp(dtorCall.getLoc(),...
[truncated]

[erichkeane]

Urgh, that conflict is way more than just a conflict :/ #193274 added a test here, but now one of MY tests causes an assert somewhere for reasons unknown (and it isn't clear if it is related to 193274).

Looking now, but in the meantime, this patch is not mergeable.

[andykaylor]

lgtm, with a few nits

[andykaylor]

This should be between "IfOp" and "ScopeOp"

[andykaylor]

Should this be static_local?

[erichkeane]

yikes! Yep, got my doc from a time when I was printing this wrong :)

[andykaylor]

It's a pre-existing issue, but this should be named something like getOrCreateRuntimeVariable to make it clear that we won't create duplicate entries.

[andykaylor]

It might be more friendly to handle this with globalOp->emitError("...")

[erichkeane]

Urgh, that conflict is way more than just a conflict :/ #193274 added a test here, but now one of MY tests causes an assert somewhere for reasons unknown (and it isn't clear if it is related to 193274).

Looking now, but in the meantime, this patch is not mergeable.

Weirdly, this was a BUILD problem! I invalidated an additional file and re-built and the problem went away (which was fortunate, since it looked like localDeclMap.find was not working!).

I'm finishing hte merge/ updating the new test check-lines that Adam added, then I'll do andy's fixes.

I would like to see @bcardosolopes and @xlauko to approve this before merging however, they both considered/discussed this solution with me, and I'd like to confirm this is what they expected from our conversation.

[xlauko]

Just a comment: as I am looking now to our control flow I literally have no idea how this should work with RegionBranchOpInterface, since this technically is entered, exited only once. I am afraid this does not conceptually fit to region-based control flow tooling that mlir has. :/ Same holds for globacl ctors/dtors.

[xlauko]

Shouldn't tls and static_local be enum? You would not need to check for uniqueness then. Alternatively you can port from incubator https://github.com/llvm/clangir/blob/63412d47b3d573ba2d7d55920541269930d4a303/clang/include/clang/CIR/Dialect/IR/CIROps.td#L92-L103

no to need write verify manually.

[erichkeane]

Huh, well, that bit of code is already here apparently! So I think I'll just use that instead.

[xlauko]

isn't this enforced by MaxSizedRegion<1>?

[erichkeane]

I'm not sure, this is existing code that I've slightly modified, but I've switched this to an assert here.

[xlauko]

Why is erase here, when it is NYI?

[erichkeane]

No good reason, I'll remove it.

[xlauko]

lets add:

let extraClassDeclaration = [{
  /// Look up the cir.global this op references. Returns null if the symbol
  /// is absent or is not a cir.global, callers that run after verification
  /// can assume non-null.
  cir::GlobalOp getReferencedGlobal(mlir::SymbolTableCollection &tables) {
    return llvm::dyn_cast_or_null<cir::GlobalOp>(
        tables.lookupNearestSymbolFrom(*this, getGlobalNameAttr()));
  }
}];

it might be handy on more places (see comments below)

[erichkeane]

Done, I'll add it and hope I get the rest of the stuff below right :)

[xlauko]

Suggested change

	mlir::Operation *op =
	symbolTable.lookupNearestSymbolFrom(*this, getGlobalNameAttr());
	if (op == nullptr || !isa<GlobalOp>(op))
	cir::GlobalOp global = getReferencedGlobal(symbolTable);
	if (!global)

[xlauko]

This is costly build of SymbolTable on each call. Instead lets use:

cir::GlobalOp globalOp = initOp.getReferencedGlobal(symbolTables);

and add cached symbol table to top + some plumming:

void LoweringPreparePass::runOnOperation() {
     mlir::SymbolTableCollection symbolTables;
...

@bcardosolopes was cleaning up symbol table lookup recently, so this might conflict? or be already present (I did not check).

[erichkeane]

Alright, got through all of the active comments, plus did 1 more fixup I thought of overnight (remove the 'ctor' region when only generating a dtor).

Please take another look and make sure I understood everything you wanted me to do, I'm not as sure about the symbol table parts, but I hope I got that right?

[erichkeane]

Done!

[erichkeane]

I'm not sure, this is existing code that I've slightly modified, but I've switched this to an assert here.

[erichkeane]

No good reason, I'll remove it.

[erichkeane]

Done, I'll add it and hope I get the rest of the stuff below right :)

[xlauko]

lgtm % nits

[xlauko]

Can you fix indent here please

[xlauko]

Sorry I probably did bad indent in suggenstion can you fix it here to match arguments, also add spece between lets here to be a bit more readable.

[xlauko]

why ??

[xlauko]

nit: I would put "Enforced by MaxSizedRegion<1>". same few lines below.