[CIR][NFC] Align dynamic dispatch of sync scopes to memory ordering

[Lancern]

This patch is a minor refactoring that aligns the dynamic dispatch path of atomic sync scopes to the dynamic dispatch path of atomic memory ordering.

[llvmorg-github-actions]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clangir

Author: Sirui Mu (Lancern)

Changes

This patch is a minor refactoring that aligns the dynamic dispatch path of atomic sync scopes to the dynamic dispatch path of atomic memory ordering.

---

Full diff: https://github.com/llvm/llvm-project/pull/206054.diff

2 Files Affected:

• (modified) clang/lib/CIR/CodeGen/CIRGenAtomic.cpp (+78-84)
• (modified) clang/lib/CIR/CodeGen/CIRGenFunction.h (+3)

diff --git a/clang/lib/CIR/CodeGen/CIRGenAtomic.cpp b/clang/lib/CIR/CodeGen/CIRGenAtomic.cpp
index 4d774761b975a..e39f6c681fb9c 100644
--- a/clang/lib/CIR/CodeGen/CIRGenAtomic.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenAtomic.cpp
@@ -890,7 +890,6 @@ static void emitAtomicOp(CIRGenFunction &cgf, AtomicExpr *expr, Address dest,
 
 // Map clang sync scope to CIR sync scope.
 static cir::SyncScopeKind convertSyncScopeToCIR(CIRGenFunction &cgf,
-                                                SourceRange range,
                                                 clang::SyncScope scope) {
   switch (scope) {
   case clang::SyncScope::SingleScope:
@@ -932,75 +931,6 @@ static cir::SyncScopeKind convertSyncScopeToCIR(CIRGenFunction &cgf,
   llvm_unreachable("unhandled sync scope");
 }
 
-static void emitAtomicOp(CIRGenFunction &cgf, AtomicExpr *expr, Address dest,
-                         Address ptr, Address val1, Address val2,
-                         Expr *isWeakExpr, Expr *failureOrderExpr, int64_t size,
-                         cir::MemOrder order,
-                         const std::optional<Expr::EvalResult> &scopeConst,
-                         mlir::Value scopeValue) {
-  std::unique_ptr<AtomicScopeModel> scopeModel = expr->getScopeModel();
-
-  if (!scopeModel) {
-    emitAtomicOp(cgf, expr, dest, ptr, val1, val2, isWeakExpr, failureOrderExpr,
-                 size, order, cir::SyncScopeKind::System);
-    return;
-  }
-
-  if (scopeConst.has_value()) {
-    cir::SyncScopeKind mappedScope = convertSyncScopeToCIR(
-        cgf, expr->getScope()->getSourceRange(),
-        scopeModel->map(scopeConst->Val.getInt().getZExtValue()));
-    emitAtomicOp(cgf, expr, dest, ptr, val1, val2, isWeakExpr, failureOrderExpr,
-                 size, order, mappedScope);
-    return;
-  }
-
-  // The sync scope is not a compile-time constant. Emit a switch statement to
-  // handle each possible value of the sync scope.
-  CIRGenBuilderTy &builder = cgf.getBuilder();
-  mlir::Location loc = cgf.getLoc(expr->getSourceRange());
-  llvm::ArrayRef<unsigned> allScopes = scopeModel->getRuntimeValues();
-  unsigned fallback = scopeModel->getFallBackValue();
-
-  cir::SwitchOp::create(
-      builder, loc, scopeValue,
-      [&](mlir::OpBuilder &, mlir::Location loc, mlir::OperationState &) {
-        mlir::Block *switchBlock = builder.getBlock();
-
-        // Default case -- use fallback scope
-        cir::SyncScopeKind fallbackScope = convertSyncScopeToCIR(
-            cgf, expr->getScope()->getSourceRange(), scopeModel->map(fallback));
-        emitDefaultCaseLabel(builder, loc);
-        emitAtomicOp(cgf, expr, dest, ptr, val1, val2, isWeakExpr,
-                     failureOrderExpr, size, order, fallbackScope);
-        builder.createBreak(loc);
-        builder.setInsertionPointToEnd(switchBlock);
-
-        // Emit a switch case for each non-fallback runtime scope value
-        for (unsigned scope : allScopes) {
-          if (scope == fallback)
-            continue;
-
-          cir::SyncScopeKind cirScope = convertSyncScopeToCIR(
-              cgf, expr->getScope()->getSourceRange(), scopeModel->map(scope));
-
-          mlir::ArrayAttr casesAttr = builder.getArrayAttr(
-              {cir::IntAttr::get(scopeValue.getType(), scope)});
-          mlir::OpBuilder::InsertPoint insertPoint;
-          cir::CaseOp::create(builder, loc, casesAttr, cir::CaseOpKind::Equal,
-                              insertPoint);
-
-          builder.restoreInsertionPoint(insertPoint);
-          emitAtomicOp(cgf, expr, dest, ptr, val1, val2, isWeakExpr,
-                       failureOrderExpr, size, order, cirScope);
-          builder.createBreak(loc);
-          builder.setInsertionPointToEnd(switchBlock);
-        }
-
-        builder.createYield(loc);
-      });
-}
-
 static std::optional<cir::MemOrder>
 getEffectiveAtomicMemOrder(cir::MemOrder oriOrder, bool isStore, bool isLoad,
                            bool isFence) {
@@ -1090,6 +1020,50 @@ static void emitAtomicExprWithDynamicMemOrder(
       });
 }
 
+static void emitAtomicExprWithDynamicSyncScope(
+    CIRGenFunction &cgf, const AtomicScopeModel *scopeModel, mlir::Value scope,
+    llvm::function_ref<void(cir::SyncScopeKind)> emitAtomicOp) {
+  CIRGenBuilderTy &builder = cgf.getBuilder();
+  llvm::ArrayRef<unsigned> allScopes = scopeModel->getRuntimeValues();
+  unsigned fallback = scopeModel->getFallBackValue();
+
+  cir::SwitchOp::create(
+      builder, scope.getLoc(), scope,
+      [&](mlir::OpBuilder &, mlir::Location loc, mlir::OperationState &) {
+        mlir::Block *switchBlock = builder.getBlock();
+
+        // Default case -- use fallback scope
+        cir::SyncScopeKind fallbackScope =
+            convertSyncScopeToCIR(cgf, scopeModel->map(fallback));
+        emitDefaultCaseLabel(builder, loc);
+        emitAtomicOp(fallbackScope);
+        builder.createBreak(loc);
+        builder.setInsertionPointToEnd(switchBlock);
+
+        // Emit a switch case for each non-fallback runtime scope value
+        for (unsigned runtimeScopeValue : allScopes) {
+          if (runtimeScopeValue == fallback)
+            continue;
+
+          cir::SyncScopeKind cirScope =
+              convertSyncScopeToCIR(cgf, scopeModel->map(runtimeScopeValue));
+
+          mlir::ArrayAttr casesAttr = builder.getArrayAttr(
+              {cir::IntAttr::get(scope.getType(), runtimeScopeValue)});
+          mlir::OpBuilder::InsertPoint insertPoint;
+          cir::CaseOp::create(builder, loc, casesAttr, cir::CaseOpKind::Equal,
+                              insertPoint);
+
+          builder.restoreInsertionPoint(insertPoint);
+          emitAtomicOp(cirScope);
+          builder.createBreak(loc);
+          builder.setInsertionPointToEnd(switchBlock);
+        }
+
+        builder.createYield(loc);
+      });
+}
+
 void CIRGenFunction::emitAtomicExprWithMemOrder(
     const Expr *memOrder, bool isStore, bool isLoad, bool isFence,
     llvm::function_ref<void(cir::MemOrder)> emitAtomicOpFn) {
@@ -1115,6 +1089,29 @@ void CIRGenFunction::emitAtomicExprWithMemOrder(
                                     emitAtomicOpFn);
 }
 
+void CIRGenFunction::emitAtomicExprWithSyncScope(
+    const AtomicScopeModel *scopeModel, const Expr *scopeExpr,
+    llvm::function_ref<void(cir::SyncScopeKind)> emitAtomicOp) {
+  if (!scopeModel || !scopeExpr) {
+    emitAtomicOp(cir::SyncScopeKind::System);
+    return;
+  }
+
+  // Try to evaluate the sync scope as a constant.
+  Expr::EvalResult scopeConst;
+  if (scopeExpr->EvaluateAsInt(scopeConst, getContext())) {
+    cir::SyncScopeKind mappedScope = convertSyncScopeToCIR(
+        *this, scopeModel->map(scopeConst.Val.getInt().getZExtValue()));
+    emitAtomicOp(mappedScope);
+    return;
+  }
+
+  // Otherwise, handle variable sync scope. Emit a switch op to match dynamic
+  // sync scope values to static sync scopes.
+  mlir::Value dynScope = emitScalarExpr(scopeExpr);
+  emitAtomicExprWithDynamicSyncScope(*this, scopeModel, dynScope, emitAtomicOp);
+}
+
 static RValue emitAtomicLibCall(CIRGenFunction &cgf, llvm::StringRef funcName,
                                 QualType resultType, CallArgList &args) {
   const CIRGenFunctionInfo &fnInfo =
@@ -1357,14 +1354,6 @@ RValue CIRGenFunction::emitAtomicExpr(AtomicExpr *e) {
   TypeInfoChars typeInfo = getContext().getTypeInfoInChars(atomicTy);
   uint64_t size = typeInfo.Width.getQuantity();
 
-  // Emit the sync scope operand, and try to evaluate it as a constant.
-  mlir::Value scope =
-      e->getScopeModel() ? emitScalarExpr(e->getScope()) : nullptr;
-  std::optional<Expr::EvalResult> scopeConst;
-  if (Expr::EvalResult eval;
-      e->getScopeModel() && e->getScope()->EvaluateAsInt(eval, getContext()))
-    scopeConst.emplace(std::move(eval));
-
   switch (e->getOp()) {
   default:
     cgm.errorNYI(e->getSourceRange(), "atomic op NYI");
@@ -1562,12 +1551,17 @@ RValue CIRGenFunction::emitAtomicExpr(AtomicExpr *e) {
                 e->getOp() == AtomicExpr::AO__scoped_atomic_load ||
                 e->getOp() == AtomicExpr::AO__scoped_atomic_load_n;
 
-  auto emitAtomicOpCallBackFn = [&](cir::MemOrder memOrder) {
-    emitAtomicOp(*this, e, dest, ptr, val1, val2, isWeakExpr, orderFailExpr,
-                 size, memOrder, scopeConst, scope);
-  };
-  emitAtomicExprWithMemOrder(e->getOrder(), isStore, isLoad, /*isFence*/ false,
-                             emitAtomicOpCallBackFn);
+  emitAtomicExprWithMemOrder(
+      e->getOrder(), isStore, isLoad, /*isFence*/ false,
+      [&](cir::MemOrder memOrder) {
+        std::unique_ptr<AtomicScopeModel> scopeModel = e->getScopeModel();
+        const Expr *scopeExpr = scopeModel ? e->getScope() : nullptr;
+        emitAtomicExprWithSyncScope(
+            scopeModel.get(), scopeExpr, [&](cir::SyncScopeKind scope) {
+              emitAtomicOp(*this, e, dest, ptr, val1, val2, isWeakExpr,
+                           orderFailExpr, size, memOrder, scope);
+            });
+      });
 
   if (resultTy->isVoidType())
     return RValue::get(nullptr);
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.h b/clang/lib/CIR/CodeGen/CIRGenFunction.h
index b6a4a277fab92..0beb5974778f3 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.h
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.h
@@ -1642,6 +1642,9 @@ class CIRGenFunction : public CIRGenTypeCache {
   void emitAtomicExprWithMemOrder(
       const Expr *memOrder, bool isStore, bool isLoad, bool isFence,
       llvm::function_ref<void(cir::MemOrder)> emitAtomicOp);
+  void emitAtomicExprWithSyncScope(
+      const AtomicScopeModel *scopeModel, const Expr *scopeExpr,
+      llvm::function_ref<void(cir::SyncScopeKind)> emitAtomicOp);
 
   mlir::LogicalResult emitAttributedStmt(const AttributedStmt &s);
 

[andykaylor]

This looks like an improvement in general, but it has an issue with changed behavior.

[andykaylor]

This change isn't quite NFC. By sinking the scope handling into this function, you've changed where (and sometimes if) it is evaluated. This example is benign, but shows the call to next_scope being sunk into the emitted switch statement and repeated:

int next_scope(void);
int load(int *p, int order) {
  return __scoped_atomic_load_n(p, order, next_scope());
}

This case shows a side-effect being dropped.

typedef struct {
  long data[3]; // 24 bytes, forcing the libcall path.
} Big;
int scope;
void reproduce(Big *src, Big *dst) {
  __scoped_atomic_load(src, dst, __ATOMIC_SEQ_CST, scope++);
}

Both of these were generated by an AI review, but I have confirmed the reported behavior change.

Grok 4.6 suggests this implementation to fix the issue:

std::unique_ptr<AtomicScopeModel> scopeModel = e->getScopeModel();
const Expr *scopeExpr = scopeModel ? e->getScope() : nullptr;
std::optional<cir::SyncScopeKind> constScope;
mlir::Value dynScope;

if (!scopeModel || !scopeExpr) {
  constScope = cir::SyncScopeKind::System;
} else if (Expr::EvalResult eval; scopeExpr->EvaluateAsInt(eval, getContext())) {
  constScope = convertSyncScopeToCIR(
      *this, scopeModel->map(eval.Val.getInt().getZExtValue()));
} else {
  dynScope = emitScalarExpr(scopeExpr);
}

emitAtomicExprWithMemOrder(
    e->getOrder(), isStore, isLoad, /*isFence*/ false,
    [&](cir::MemOrder memOrder) {
      if (constScope) {
        emitAtomicOp(..., memOrder, *constScope);
        return;
      }
      emitAtomicExprWithDynamicSyncScope(
          *this, scopeModel.get(), dynScope, [&](cir::SyncScopeKind scope) {
            emitAtomicOp(..., memOrder, scope);
          });
    });

[Lancern]

I think the problem is that the lib-call path does not evaluate the scope argument even for those scoped operations, because the library functions would omit the scope anyway. Maybe we should just add appropriate emission to emitLibCallForAtomicExpr?

In emitLibCallForAtomicExpr:

if (e->getScopeModel()) {
  if (const Expr *scopeExpr = e->getScope()) {
    Expr::EvalResult scopeConst;
    if (!scopeExpr->EvaluateAsInt(scopeConst, cgf.getContext()))
      cgf.emitScalarExpr(scopeExpr);
  }
}

[andykaylor]

My understanding is that it's caused by sinking the emitAtomicOp call into the lambdas here. There are paths where the lambda is just not called.

[Lancern]

Since the primary purpose of this PR is to group together the evaluation and dynamic dispatch of order and scope, I think that instead of sinking scope handling, we could promote the handling of order, something like:

RValue CIRGenFunction::emitAtomicExpr(AtomicExpr *e) {
  // ...

  Expr *orderExpr = e->getOrder();
  Expr *scopeExpr = e->getScope();

  Expr::EvalResult order, scope;
  mlir::Value orderValue, scopeValue;
  if (!orderExpr->EvaluateAsInt(order, getContext()))
    orderValue = emitScalarExpr(orderExpr);
  if (!scopeExpr->EvaluateAsInt(scope, getContext()))
    scopeValue = emitScalarExpr(scopeExpr);

  // ...

  emitAtomicExprWithMemOrder(order, orderValue, /*...*/,
    [&](cir::MemOrder order) {
      emitAtomicExprWithSyncScope(scope, scopeValue, /*...*/,
        [&](cir::SyncScopeKind scope) {
          emitAtomicOp(/*...*/, order, scope);
        }
      );
    }
  );
}

Does this approach sound OK to you?

[andykaylor]

Yes, that should be fine.

[Lancern]

Updated.

[Lancern]

Added scope emission to the lib-call path.

[github-actions]

🐧 Linux x64 Test Results

• 95565 tests passed
• 4378 tests skipped

✅ The build succeeded and all tests passed.

[bcardosolopes]

Should this get an update? :)

[Lancern]

Rebased.