Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] constexpr atomic builtins (__c11_atomic_OP and __atomic_OP) #98756

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

hanickadot
Copy link
Contributor

This implements clang support for P3309 constexpr std::atomic & std::atomic_ref (currently in LWG) by allowing constant evaluation of clang's __c11_atomic_OP and GCC's __atomic_OP builtins.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 13, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 13, 2024

@llvm/pr-subscribers-clang

Author: Hana Dusíková (hanickadot)

Changes

This implements clang support for P3309 constexpr std::atomic & std::atomic_ref (currently in LWG) by allowing constant evaluation of clang's __c11_atomic_OP and GCC's __atomic_OP builtins.


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

4 Files Affected:

  • (modified) clang/include/clang/Basic/Builtins.td (+42-42)
  • (modified) clang/lib/AST/ExprConstant.cpp (+547-6)
  • (added) clang/test/SemaCXX/atomic-constexpr-c11-builtins.cpp (+288)
  • (added) clang/test/SemaCXX/atomic-constexpr-gcc-builtins.cpp (+494)
diff --git a/clang/include/clang/Basic/Builtins.td b/clang/include/clang/Basic/Builtins.td
index f5b15cf90d1f8..0716cf02f5110 100644
--- a/clang/include/clang/Basic/Builtins.td
+++ b/clang/include/clang/Basic/Builtins.td
@@ -1682,97 +1682,97 @@ def SyncSwapN : Builtin, SyncBuiltinsTemplate {
 // C11 _Atomic operations for <stdatomic.h>.
 def C11AtomicInit : AtomicBuiltin {
   let Spellings = ["__c11_atomic_init"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicLoad : AtomicBuiltin {
   let Spellings = ["__c11_atomic_load"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicStore : AtomicBuiltin {
   let Spellings = ["__c11_atomic_store"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicExchange : AtomicBuiltin {
   let Spellings = ["__c11_atomic_exchange"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicCompareExchangeStrong : AtomicBuiltin {
   let Spellings = ["__c11_atomic_compare_exchange_strong"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicCompareExchangeWeak : AtomicBuiltin {
   let Spellings = ["__c11_atomic_compare_exchange_weak"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicFetchAdd : AtomicBuiltin {
   let Spellings = ["__c11_atomic_fetch_add"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicFetchSub : AtomicBuiltin {
   let Spellings = ["__c11_atomic_fetch_sub"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicFetchAnd : AtomicBuiltin {
   let Spellings = ["__c11_atomic_fetch_and"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicFetchOr : AtomicBuiltin {
   let Spellings = ["__c11_atomic_fetch_or"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicFetchXor : AtomicBuiltin {
   let Spellings = ["__c11_atomic_fetch_xor"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicFetchNand : AtomicBuiltin {
   let Spellings = ["__c11_atomic_fetch_nand"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicFetchMax : AtomicBuiltin {
   let Spellings = ["__c11_atomic_fetch_max"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicFetchMin : AtomicBuiltin {
   let Spellings = ["__c11_atomic_fetch_min"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def C11AtomicThreadFence : Builtin {
   let Spellings = ["__c11_atomic_thread_fence"];
-  let Attributes = [NoThrow];
+  let Attributes = [NoThrow, Constexpr];
   let Prototype = "void(int)";
 }
 
 def C11AtomicSignalFence : Builtin {
   let Spellings = ["__c11_atomic_signal_fence"];
-  let Attributes = [NoThrow];
+  let Attributes = [NoThrow, Constexpr];
   let Prototype = "void(int)";
 }
 
@@ -1785,157 +1785,157 @@ def C11AtomicIsLockFree : Builtin {
 // GNU atomic builtins.
 def AtomicLoad : AtomicBuiltin {
   let Spellings = ["__atomic_load"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicLoadN : AtomicBuiltin {
   let Spellings = ["__atomic_load_n"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicStore : AtomicBuiltin {
   let Spellings = ["__atomic_store"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicStoreN : AtomicBuiltin {
   let Spellings = ["__atomic_store_n"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicExchange : AtomicBuiltin {
   let Spellings = ["__atomic_exchange"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicExchangeN : AtomicBuiltin {
   let Spellings = ["__atomic_exchange_n"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicCompareExchange : AtomicBuiltin {
   let Spellings = ["__atomic_compare_exchange"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicCompareExchangeN : AtomicBuiltin {
   let Spellings = ["__atomic_compare_exchange_n"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicFetchAdd : AtomicBuiltin {
   let Spellings = ["__atomic_fetch_add"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicFetchSub : AtomicBuiltin {
   let Spellings = ["__atomic_fetch_sub"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicFetchAnd : AtomicBuiltin {
   let Spellings = ["__atomic_fetch_and"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicFetchOr : AtomicBuiltin {
   let Spellings = ["__atomic_fetch_or"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicFetchXor : AtomicBuiltin {
   let Spellings = ["__atomic_fetch_xor"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicFetchNand : AtomicBuiltin {
   let Spellings = ["__atomic_fetch_nand"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicAddFetch : AtomicBuiltin {
   let Spellings = ["__atomic_add_fetch"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicSubFetch : AtomicBuiltin {
   let Spellings = ["__atomic_sub_fetch"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicAndFetch : AtomicBuiltin {
   let Spellings = ["__atomic_and_fetch"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicOrFetch : AtomicBuiltin {
   let Spellings = ["__atomic_or_fetch"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicXorFetch : AtomicBuiltin {
   let Spellings = ["__atomic_xor_fetch"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicMaxFetch : AtomicBuiltin {
   let Spellings = ["__atomic_max_fetch"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicMinFetch : AtomicBuiltin {
   let Spellings = ["__atomic_min_fetch"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicNandFetch : AtomicBuiltin {
   let Spellings = ["__atomic_nand_fetch"];
-  let Attributes = [CustomTypeChecking];
+  let Attributes = [CustomTypeChecking, Constexpr];
   let Prototype = "void(...)";
 }
 
 def AtomicTestAndSet : Builtin {
   let Spellings = ["__atomic_test_and_set"];
-  let Attributes = [NoThrow];
+  let Attributes = [NoThrow, Constexpr];
   let Prototype = "bool(void volatile*, int)";
 }
 
 def AtomicClear : Builtin {
   let Spellings = ["__atomic_clear"];
-  let Attributes = [NoThrow];
+  let Attributes = [NoThrow, Constexpr];
   let Prototype = "void(void volatile*, int)";
 }
 
 def AtomicThreadFence : Builtin {
   let Spellings = ["__atomic_thread_fence"];
-  let Attributes = [NoThrow];
+  let Attributes = [NoThrow, Constexpr];
   let Prototype = "void(int)";
 }
 
 def AtomicSignalFence : Builtin {
   let Spellings = ["__atomic_signal_fence"];
-  let Attributes = [NoThrow];
+  let Attributes = [NoThrow, Constexpr];
   let Prototype = "void(int)";
 }
 
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 0aeac9d03eed3..c472b4b998aa3 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -1900,6 +1900,17 @@ static bool EvaluateFixedPoint(const Expr *E, APFixedPoint &Result,
 // Misc utilities
 //===----------------------------------------------------------------------===//
 
+static bool isOnePastTheEndOfCompleteObject(const ASTContext &Ctx,
+                                            const LValue &LV);
+
+enum class SizeOfType {
+  SizeOf,
+  DataSizeOf,
+};
+
+static bool HandleSizeof(EvalInfo &Info, SourceLocation Loc, QualType Type,
+                         CharUnits &Size, SizeOfType SOT = SizeOfType::SizeOf);
+
 /// Negate an APSInt in place, converting it to a signed form if necessary, and
 /// preserving its value (by extending by up to one bit as needed).
 static void negateAsSigned(APSInt &Int) {
@@ -3222,14 +3233,9 @@ static bool HandleLValueIndirectMember(EvalInfo &Info, const Expr *E,
   return true;
 }
 
-enum class SizeOfType {
-  SizeOf,
-  DataSizeOf,
-};
-
 /// Get the size of the given type in char units.
 static bool HandleSizeof(EvalInfo &Info, SourceLocation Loc, QualType Type,
-                         CharUnits &Size, SizeOfType SOT = SizeOfType::SizeOf) {
+                         CharUnits &Size, SizeOfType SOT) {
   // sizeof(void), __alignof__(void), sizeof(function) = 1 as a gcc
   // extension.
   if (Type->isVoidType() || Type->isFunctionType()) {
@@ -7884,6 +7890,522 @@ class ExprEvaluatorBase
     return StmtVisitorTy::Visit(Source);
   }
 
+  static bool EvaluateAtomicOrderToIgnore(const AtomicExpr *E, EvalInfo &Info) {
+    // we ignore results, but we need to evaluate them
+    [[maybe_unused]] APSInt OrderIgnoredResult;
+
+    const Expr *OrderSuccess = E->getOrder();
+    if (!EvaluateInteger(OrderSuccess, OrderIgnoredResult, Info))
+      return false;
+
+    if (E->isCmpXChg()) {
+      const Expr *OrderFail = E->getOrderFail();
+      if (!EvaluateInteger(OrderFail, OrderIgnoredResult, Info))
+        return false;
+    }
+
+    return true;
+  }
+
+  static bool EvaluateAtomicWeakToIgnore(const AtomicExpr *E, EvalInfo &Info) {
+    // we ignore results, but we need to evaluate them
+    [[maybe_unused]] APSInt WeakIgnoredResult;
+
+    if (E->getOp() == AtomicExpr::AO__atomic_compare_exchange_n ||
+        E->getOp() == AtomicExpr::AO__atomic_compare_exchange) {
+      const Expr *Weak = E->getWeak();
+      if (!EvaluateInteger(Weak, WeakIgnoredResult, Info))
+        return false;
+    }
+
+    return true;
+  }
+
+  static bool LoadAtomicValue(const AtomicExpr *E, APValue &Result,
+                              EvalInfo &Info) {
+    LValue AtomicStorageLV;
+
+    if (!EvaluatePointer(E->getPtr(), AtomicStorageLV, Info))
+      return false;
+
+    return handleLValueToRValueConversion(Info, E->getPtr(), E->getValueType(),
+                                          AtomicStorageLV, Result);
+  }
+
+  static bool StoreValueIntoResultPointer(Expr *ResultPtr,
+                                          APValue &ValueToStore,
+                                          EvalInfo &Info) {
+    // TODO check it must be a pointer
+    assert(ResultPtr->getType()->isPointerType());
+    QualType PointeeTy = ResultPtr->getType()->getPointeeType();
+    LValue PointeeLV;
+
+    if (!EvaluatePointer(ResultPtr, PointeeLV, Info))
+      return false;
+
+    return handleAssignment(Info, ResultPtr, PointeeLV, PointeeTy,
+                            ValueToStore);
+  }
+
+  static bool LoadAtomicValueInto(const AtomicExpr *E, EvalInfo &Info) {
+    APValue LocalResult;
+
+    if (!LoadAtomicValue(E, LocalResult, Info))
+      return false;
+
+    if (!StoreValueIntoResultPointer(E->getVal1(), LocalResult, Info))
+      return false;
+
+    return true;
+  }
+
+  static bool StoreAtomicValue(const AtomicExpr *E, EvalInfo &Info) {
+    LValue AtomicStorageLV;
+
+    if (!EvaluatePointer(E->getPtr(), AtomicStorageLV, Info))
+      return false;
+
+    APValue ProvidedValue;
+
+    // GCC's atomic_store takes pointer to value, not value itself
+    if (E->getOp() == AtomicExpr::AO__atomic_store) {
+      LValue ProvidedLV;
+      if (!EvaluatePointer(E->getVal1(), ProvidedLV, Info))
+        return false;
+
+      if (!handleLValueToRValueConversion(Info, E->getVal1(),
+                                          E->getVal1()->getType(), ProvidedLV,
+                                          ProvidedValue))
+        return false;
+
+    } else {
+      if (!Evaluate(ProvidedValue, Info, E->getVal1()))
+        return false;
+    }
+    if (!handleAssignment(Info, E, AtomicStorageLV, E->getValueType(),
+                          ProvidedValue))
+      return false;
+
+    return true;
+  }
+
+  static bool ExchangeAtomicValueInto(const AtomicExpr *E, EvalInfo &Info) {
+    assert(E->getOp() == AtomicExpr::AO__atomic_exchange);
+    // implementation of GCC's exchange (non _n version)
+    LValue AtomicStorageLV;
+    if (!EvaluatePointer(E->getPtr(), AtomicStorageLV, Info))
+      return false;
+
+    // read previous value
+    APValue PreviousValue;
+    if (!handleLValueToRValueConversion(Info, E->getPtr(), E->getValueType(),
+                                        AtomicStorageLV, PreviousValue))
+      return false;
+
+    // get provided value from argument (pointer)
+    LValue ProvidedLV;
+    if (!EvaluatePointer(E->getVal1(), ProvidedLV, Info))
+      return false;
+
+    APValue ProvidedValue;
+    if (!handleLValueToRValueConversion(Info, E->getVal1(),
+                                        E->getVal1()->getType(), ProvidedLV,
+                                        ProvidedValue))
+      return false;
+
+    // store provided value to atomic value
+    if (!handleAssignment(Info, E, AtomicStorageLV, E->getValueType(),
+                          ProvidedValue))
+      return false;
+
+    // store previous value in output pointer
+    if (!StoreValueIntoResultPointer(E->getVal2(), PreviousValue, Info))
+      return false;
+
+    return true;
+  }
+
+  static bool FetchAtomicOp(const AtomicExpr *E, APValue &Result,
+                            EvalInfo &Info, bool StoreToResultAfter) {
+    // read atomic
+    LValue AtomicStorageLV;
+    QualType AtomicValueTy = E->getValueType();
+    if (!EvaluatePointer(E->getPtr(), AtomicStorageLV, Info))
+      return false;
+
+    APValue CurrentValue;
+    if (!handleLValueToRValueConversion(Info, E->getPtr(), E->getType(),
+                                        AtomicStorageLV, CurrentValue))
+      return false;
+
+    // store current value for fetch-OP operations
+    if (!StoreToResultAfter)
+      Result = CurrentValue;
+
+    // read argument for fetch OP
+    APValue ArgumentVal;
+    if (!Evaluate(ArgumentVal, Info, E->getVal1()))
+      return false;
+
+    // calculate new value
+    APValue Replacement;
+    if (AtomicValueTy->isIntegralOrEnumerationType()) {
+      // both arguments are integers
+      const APSInt AtomicInt = CurrentValue.getInt();
+      const APSInt ArgumentInt = ArgumentVal.getInt();
+
+      switch (E->getOp()) {
+      case AtomicExpr::AO__c11_atomic_fetch_add:
+      case AtomicExpr::AO__atomic_fetch_add:
+      case AtomicExpr::AO__atomic_add_fetch:
+        Replacement = APValue(AtomicInt + ArgumentInt);
+        break;
+      case AtomicExpr::AO__c11_atomic_fetch_sub:
+      case AtomicExpr::AO__atomic_fetch_sub:
+      case AtomicExpr::AO__atomic_sub_fetch:
+        Replacement = APValue(AtomicInt - ArgumentInt);
+        break;
+      case AtomicExpr::AO__c11_atomic_fetch_and:
+      case AtomicExpr::AO__atomic_fetch_and:
+      case AtomicExpr::AO__atomic_and_fetch:
+        Replacement = APValue(AtomicInt & ArgumentInt);
+        break;
+      case AtomicExpr::AO__c11_atomic_fetch_or:
+      case AtomicExpr::AO__atomic_fetch_or:
+      case AtomicExpr::AO__atomic_or_fetch:
+        Replacement = APValue(AtomicInt | ArgumentInt);
+        break;
+      case AtomicExpr::AO__c11_atomic_fetch_xor:
+      case AtomicExpr::AO__atomic_fetch_xor:
+      case AtomicExpr::AO__atomic_xor_fetch:
+        Replacement = APValue(AtomicInt ^ ArgumentInt);
+        break;
+      case AtomicExpr::AO__c11_atomic_fetch_nand:
+      case AtomicExpr::AO__atomic_fetch_nand:
+      case AtomicExpr::AO__atomic_nand_fetch:
+        Replacement = APValue(~(AtomicInt & ArgumentInt));
+        break;
+      case AtomicExpr::AO__c11_atomic_fetch_max:
+      case AtomicExpr::AO__atomic_fetch_max:
+      case AtomicExpr::AO__atomic_max_fetch:
+        Replacement =
+            APValue((AtomicInt > ArgumentInt) ? AtomicInt : ArgumentInt);
+        break;
+      case AtomicExpr::AO__c11_atomic_fetch_min:
+      case AtomicExpr::AO__atomic_fetch_min:
+      case AtomicExpr::AO__atomic_min_fetch:
+        Replacement =
+            APValue((AtomicInt < ArgumentInt) ? AtomicInt : ArgumentInt);
+        break;
+      default:
+        return false;
+      }
+    } else if (AtomicValueTy->isRealFloatingType()) {
+      // both arguments are float operations
+      const llvm::RoundingMode RM = getActiveRoundingMode(Info, E);
+      APFloat AtomicFlt = CurrentValue.getFloat();
+      const APFloat ArgumentFlt = ArgumentVal.getFloat();
+      APFloat::opStatus St;
+
+      switch (E->getOp()) {
+      case AtomicExpr::AO__c11_atomic_fetch_add: // GCC atomics doesn't support
+                                                 // floats
+        St = AtomicFlt.add(ArgumentFlt, RM);
+        Replacement = APValue(AtomicFlt);
+        break;
+      case AtomicExpr::AO__c11_atomic_fetch_sub:
+        St = AtomicFlt.subtract(ArgumentFlt, RM);
+        Replacement = APValue(AtomicFlt);
+        break;
+      default:
+        return false;
+      }
+
+      if (!checkFloatingPointResult(Info, E, St))
+        return false;
+
+    } else if (AtomicValueTy->isPointerType()) {
+      // pointer + int arguments
+      LValue AtomicPtr;
+      AtomicPtr.setFrom(Info.Ctx, CurrentValue);
+
+      APSInt ArgumentInt = ArgumentVal.getInt();
+
+      // calculate size of pointee object
+      CharUnits SizeOfPointee;
+      if (!HandleSizeof(Info, E->getExprLoc(), AtomicValueTy->getPointeeType(),
+                        SizeOfPointee))
+        return false;
+
+      // GCC's atomic_fetch add/sub compute new pointer by bytes and not
+      // sizeof(T)
+      switch (E->getOp()) {
+      case AtomicExpr::AO__atomic_fetch_add:
+      case AtomicExpr::AO__atomic_add_fetch:
+      case AtomicExpr::AO__atomic_fetch_sub:
+      case AtomicExpr::AO__atomic_sub_fetch: {
+        const auto sizeOfOneItem =
+            APSInt(APInt(ArgumentInt.getBitWidth(), SizeOfPointee.getQuantity(),
+                         false),
+                   false);
+        // incrementing pointer by size which is not dividable by pointee size
+        // is UB and therefore disallowed
+        if ((ArgumentInt % sizeOfOneItem) != 0)
+          return false;
+
+        ArgumentInt /= sizeOfOneItem;
+      } break;
+  ...
[truncated]

@hanickadot hanickadot force-pushed the P3309-constexpr-atomic-clang branch from e42b4a8 to 76fe494 Compare July 13, 2024 18:30
Copy link

github-actions bot commented Jul 13, 2024

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

@hanickadot hanickadot force-pushed the P3309-constexpr-atomic-clang branch from 000b6a5 to f308fee Compare July 13, 2024 18:59
@hanickadot hanickadot force-pushed the P3309-constexpr-atomic-clang branch from f308fee to e21d8ac Compare July 29, 2024 19:13
@hanickadot hanickadot force-pushed the P3309-constexpr-atomic-clang branch from e21d8ac to bfb43ab Compare January 6, 2025 19:42
@hanickadot
Copy link
Contributor Author

hanickadot commented Jan 6, 2025

It's broken after rebase, fixing it now... fixed

@hanickadot hanickadot force-pushed the P3309-constexpr-atomic-clang branch from bfb43ab to 7d65b13 Compare January 6, 2025 20:06
clang/lib/AST/ExprConstant.cpp Outdated Show resolved Hide resolved
EvalInfo &Info, bool StoreToResultAfter);
static bool EvaluateAtomicCompareExchange(const AtomicExpr *E, APValue &Result,
EvalInfo &Info);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mos of those functions are pretty short, does it improve readability to inline them into the switch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I inlined some small functions in bigger, but I'm not fan of long switches. I personally found them one of the reasons I struggle understand what's happening in ExprConstant.cpp as I found myself scrolling up/down. So I'm keeping them in their own functions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion either way currently, but because this is constant expression evaluation and C++ already makes processing headers painfully slow, we prefer reduced readability if it measurably improves performance in this case. So measuring overhead may not be a bad idea.

clang/lib/AST/ExprConstant.cpp Outdated Show resolved Hide resolved
@hanickadot hanickadot force-pushed the P3309-constexpr-atomic-clang branch from d602645 to 13dcdff Compare January 8, 2025 11:04
…are now full sentences. Add Diagnostics for GCC's atomic misaligned op. Reuse pointer comparison code instead of duplicating.
@hanickadot hanickadot force-pushed the P3309-constexpr-atomic-clang branch from 13dcdff to 1994a55 Compare January 8, 2025 11:11
@hanickadot hanickadot requested a review from tbaederr January 8, 2025 12:22
Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this! Please be sure to also add release notes to clang/docs/ReleaseNotes.rst so users know about the new functionality. Also, this should update clang/www/cxx_status.html to mark that we now implement P3309.

let Prototype = "void(...)";
}

def C11AtomicLoad : AtomicBuiltin {
let Spellings = ["__c11_atomic_load"];
let Attributes = [CustomTypeChecking];
let Attributes = [CustomTypeChecking, Constexpr];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about changing the C11 atomic builtins for a C++26 feature; I think there are more changes needed as a result. For example, changing this to be constexpr means we now run the risk of accepting this code in C: https://godbolt.org/z/hc3oqYx6b

We likely are missing test coverage for this kind of thing in C, so I would recommend 1) add test coverage to this PR and if it breaks as a result of these changes, then 2) I would probably add some getLangOpts().CPlusPlus checks in the constant expression evaluator. We could invent new tablegen to say "this is constexpr only in C++", but it's not clear to me whether there will be enough builtins that need such functionality or not, but that's another (cleaner, but a heavier lift for a new contributor) option.

Hmm, but I just remembered that __has_constexpr_builtin (https://clang.llvm.org/docs/LanguageExtensions.html#has-constexpr-builtin) is a thing, so we may need that tablegen solution after all because otherwise we change the behavior here in confusing ways: https://godbolt.org/z/qGY575MGr FWIW, here's an example of some recent changes to tablegen that you could model your changes after: https://github.com/llvm/llvm-project/pull/91894/files

@@ -1847,133 +1847,133 @@ def C11AtomicIsLockFree : Builtin {
// GNU atomic builtins.
def AtomicLoad : AtomicBuiltin {
let Spellings = ["__atomic_load"];
let Attributes = [CustomTypeChecking];
let Attributes = [CustomTypeChecking, Constexpr];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When removing the constexpr-ness from these functions in C, please also make sure __c11_atomic_is_lock_free is tested as well; that was already marked constexpr and that might be a bug.

EvalInfo &Info, bool StoreToResultAfter);
static bool EvaluateAtomicCompareExchange(const AtomicExpr *E, APValue &Result,
EvalInfo &Info);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion either way currently, but because this is constant expression evaluation and C++ already makes processing headers painfully slow, we prefer reduced readability if it measurably improves performance in this case. So measuring overhead may not be a bad idea.

@@ -8075,6 +8089,64 @@ class ExprEvaluatorBase
return StmtVisitorTy::Visit(Source);
}

bool VisitAtomicExpr(const AtomicExpr *E) {
if (!EvaluateAtomicOrder(E, Info))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional that there's no check for C++26 anywhere to reject these uses in earlier language modes? If you intend for this to be an extension in older modes, that seems reasonable to me, but you should update clang/docs/LanguageExtensions.rst to explicitly document the extension, add the usual pre-compat/extension warnings, tests for those two new diagnostics, and add a RUN line for an older language mode to your larger functionality tests.

clang/lib/AST/ExprConstant.cpp Show resolved Hide resolved
clang/lib/AST/ExprConstant.cpp Outdated Show resolved Hide resolved
clang/lib/AST/ExprConstant.cpp Outdated Show resolved Hide resolved
clang/test/SemaCXX/atomic-constexpr-c11-builtins.cpp Outdated Show resolved Hide resolved
clang/test/SemaCXX/atomic-constexpr-gcc-builtins.cpp Outdated Show resolved Hide resolved
…ory Ordering arguments of atomic's operations, support `void *` pointer for GCC's atomic_fetch_add/sub and atomic_add/sub_fetch
…ded, as they are expressions and not functions"

This reverts commit 9de180f.
@hanickadot
Copy link
Contributor Author

Also, this should update clang/www/cxx_status.html to mark that we now implement P3309.

I'm not sure ... should it go there if it's only a library feature, which needs the builtins to work? __builtin_is_within_lifetime didn't go to the release notes too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants