Relax assert in ExprConstant to a return None.

[JonChesterfield]

Summary

Fixes a compiler assert on passing a compile time integer to atomic builtins.

Assert introduced in D61522
Function changed from ->bool to ->Optional in D76646
Simplifies call sites to getIntegerConstantExpr to elide the now-redundant
isValueDependent checks.

[jdoerfert]

This makes sense, any reason we don't remove the check at the call sites right away?

[JonChesterfield]

In D112159#3075703, @jdoerfert wrote:

This makes sense, any reason we don't remove the check at the call sites right away?

There are ~85 of them, will start looking through now.

[JonChesterfield]

• fix up call sites

[JonChesterfield]

• fix up call sites

[JonChesterfield]

• drop spurious nl

[JonChesterfield]

Patched up the call sites that are straightforward. There are ~4 calls in SemaOpenMP where an !isValueDependent() could be dropped if it is safe to call PerformContextualImplicitConversion on such a value, which it probably is, but it's hard to be certain. There are a few places where Diag() calls would be changed by dropping the call, those are also left unchanged here.

[JonChesterfield]

clang/lib/Sema/SemaExprCXX.cpp

2137

Shame about the whitespace noise. getIntegerConstantExpr guards the block and is dominated by isValueDependent.

clang/lib/Sema/SemaTemplateDeduction.cpp

2171–2172

this parses as !ArgExpr->isValueDependent() && ArgExpr->getIntegerConstantExpr(S.Context) && ...

[aaron.ballman]

I checked the call sites to see if any were missed and I did not spot uses that should also be changed. LGTM, thanks!

[JonChesterfield]

Thanks! Lots of call sites to check, appreciate the check

clang/lib/AST/ExprConstant.cpp

if (D.Kind != IK_ICE) {
     return false;
   }
   return true;
 }
 
 Optional<llvm::APSInt> Expr::getIntegerConstantExpr(const ASTContext &Ctx,
                                                     SourceLocation *Loc,
                                                     bool isEvaluated) const {
-  assert(!isValueDependent() &&
-         "Expression evaluator can't be called on a dependent expression.");
+  if (isValueDependent()) {
+    // Expression evaluator can't succeed on a dependent expression.
+    return None;
+  }
 
   APSInt Value;
 
   if (Ctx.getLangOpts().CPlusPlus11) {
     if (EvaluateCPlusPlus11IntegralConstantExpr(Ctx, this, &Value, Loc))
       return Value;
     return None;
   }

clang/lib/Sema/SemaAttr.cpp

void Sema::ActOnPragmaPack(SourceLocation PragmaLoc, PragmaMsStackAction Action,
   AlignPackInfo::Mode ModeVal = CurVal.getAlignMode();
 
   if (Alignment) {
     Optional<llvm::APSInt> Val;
     Val = Alignment->getIntegerConstantExpr(Context);
 
     // pack(0) is like pack(), which just works out since that is what
     // we use 0 for in PackAttr.
-    if (Alignment->isTypeDependent() || Alignment->isValueDependent() || !Val ||
+    if (Alignment->isTypeDependent() || !Val ||
         !(*Val == 0 || Val->isPowerOf2()) || Val->getZExtValue() > 16) {
       Diag(PragmaLoc, diag::warn_pragma_pack_invalid_alignment);
       return; // Ignore
     }
 
     if (IsXLPragma && *Val == 0) {
       // pack(0) does not work out with XL.
       Diag(PragmaLoc, diag::err_pragma_pack_invalid_alignment);

clang/lib/Sema/SemaDeclAttr.cpp

 ///
 /// Negative argument is implicitly converted to unsigned, unless
 /// \p StrictlyUnsigned is true.
 template <typename AttrInfo>
 static bool checkUInt32Argument(Sema &S, const AttrInfo &AI, const Expr *Expr,
                                 uint32_t &Val, unsigned Idx = UINT_MAX,
                                 bool StrictlyUnsigned = false) {
   Optional<llvm::APSInt> I = llvm::APSInt(32);
-  if (Expr->isTypeDependent() || Expr->isValueDependent() ||
+  if (Expr->isTypeDependent() ||
       !(I = Expr->getIntegerConstantExpr(S.Context))) {
     if (Idx != UINT_MAX)
       S.Diag(getAttrLoc(AI), diag::err_attribute_argument_n_type)
           << &AI << Idx << AANT_ArgumentIntegerConstant
           << Expr->getSourceRange();
     else
       S.Diag(getAttrLoc(AI), diag::err_attribute_argument_type)
           << &AI << AANT_ArgumentIntegerConstant << Expr->getSourceRange();
static bool checkFunctionOrMethodParameterIndex(
   // Parameters are counted from one.
   bool HP = hasFunctionProto(D);
   bool HasImplicitThisParam = isInstanceMethod(D);
   bool IV = HP && isFunctionOrMethodVariadic(D);
   unsigned NumParams =
       (HP ? getFunctionOrMethodNumParams(D) : 0) + HasImplicitThisParam;
 
   Optional<llvm::APSInt> IdxInt;
-  if (IdxExpr->isTypeDependent() || IdxExpr->isValueDependent() ||
+  if (IdxExpr->isTypeDependent() ||
       !(IdxInt = IdxExpr->getIntegerConstantExpr(S.Context))) {
     S.Diag(getAttrLoc(AI), diag::err_attribute_argument_n_type)
         << &AI << AttrArgNum << AANT_ArgumentIntegerConstant
         << IdxExpr->getSourceRange();
     return false;
   }
 
   unsigned IdxSource = IdxInt->getLimitedValue(UINT_MAX);
static void handleBlocksAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   D->addAttr(::new (S.Context) BlocksAttr(S.Context, AL, type));
 }
 
 static void handleSentinelAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
   unsigned sentinel = (unsigned)SentinelAttr::DefaultSentinel;
   if (AL.getNumArgs() > 0) {
     Expr *E = AL.getArgAsExpr(0);
     Optional<llvm::APSInt> Idx = llvm::APSInt(32);
-    if (E->isTypeDependent() || E->isValueDependent() ||
-        !(Idx = E->getIntegerConstantExpr(S.Context))) {
+    if (E->isTypeDependent() || !(Idx = E->getIntegerConstantExpr(S.Context))) {
       S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type)
           << AL << 1 << AANT_ArgumentIntegerConstant << E->getSourceRange();
       return;
     }
 
     if (Idx->isSigned() && Idx->isNegative()) {
       S.Diag(AL.getLoc(), diag::err_attribute_sentinel_less_than_zero)
         << E->getSourceRange();
       return;
     }
 
     sentinel = Idx->getZExtValue();
   }
 
   unsigned nullPos = (unsigned)SentinelAttr::DefaultNullPos;
   if (AL.getNumArgs() > 1) {
     Expr *E = AL.getArgAsExpr(1);
     Optional<llvm::APSInt> Idx = llvm::APSInt(32);
-    if (E->isTypeDependent() || E->isValueDependent() ||
-        !(Idx = E->getIntegerConstantExpr(S.Context))) {
+    if (E->isTypeDependent() || !(Idx = E->getIntegerConstantExpr(S.Context))) {
       S.Diag(AL.getLoc(), diag::err_attribute_argument_n_type)
           << AL << 2 << AANT_ArgumentIntegerConstant << E->getSourceRange();
       return;
     }
     nullPos = Idx->getZExtValue();
 
     if ((Idx->isSigned() && Idx->isNegative()) || nullPos > 1) {
       // FIXME: This error message could be improved, it would be nice

clang/lib/Sema/SemaExprCXX.cpp

if (ArraySize && *ArraySize && !(*ArraySize)->isTypeDependent()) {
 
     if (!SizeType->isIntegralOrUnscopedEnumerationType())
       return ExprError();
 
     // C++98 [expr.new]p7:
     //   The expression in a direct-new-declarator shall have integral type
     //   with a non-negative value.
     //
     // Let's see if this is a constant < 0. If so, we reject it out of hand,
     // per CWG1464. Otherwise, if it's not a constant, we must have an
     // unparenthesized array type.
-    if (!(*ArraySize)->isValueDependent()) {
-      // We've already performed any required implicit conversion to integer or
-      // unscoped enumeration type.
-      // FIXME: Per CWG1464, we are required to check the value prior to
-      // converting to size_t. This will never find a negative array size in
-      // C++14 onwards, because Value is always unsigned here!
-      if (Optional<llvm::APSInt> Value =
-              (*ArraySize)->getIntegerConstantExpr(Context)) {
-        if (Value->isSigned() && Value->isNegative()) {
-          return ExprError(Diag((*ArraySize)->getBeginLoc(),
-                                diag::err_typecheck_negative_array_size)
-                           << (*ArraySize)->getSourceRange());
-        }
+
 
-        if (!AllocType->isDependentType()) {
-          unsigned ActiveSizeBits = ConstantArrayType::getNumAddressingBits(
-              Context, AllocType, *Value);
-          if (ActiveSizeBits > ConstantArrayType::getMaxSizeBits(Context))
-            return ExprError(
-                Diag((*ArraySize)->getBeginLoc(), diag::err_array_too_large)
-                << toString(*Value, 10) << (*ArraySize)->getSourceRange());
-        }
+        unsigned ActiveSizeBits =
+            ConstantArrayType::getNumAddressingBits(Context, AllocType, *Value);
 
-        KnownArraySize = Value->getZExtValue();
-      } else if (TypeIdParens.isValid()) {
-        // Can't have dynamic array size when the type-id is in parentheses.
-        Diag((*ArraySize)->getBeginLoc(), diag::ext_new_paren_array_nonconst)
-            << (*ArraySize)->getSourceRange()
-            << FixItHint::CreateRemoval(TypeIdParens.getBegin())
-            << FixItHint::CreateRemoval(TypeIdParens.getEnd());
 
-        TypeIdParens = SourceRange();
-      }
-    }
 
     // Note that we do *not* convert the argument in any way.  It can
     // be signed, larger than size_t, whatever.
   }
 
   FunctionDecl *OperatorNew = nullptr;
   FunctionDecl *OperatorDelete = nullptr;
   unsigned Alignment =

clang/lib/Sema/SemaTemplateDeduction.cpp

case Type::DependentSizedMatrix: {
                 return Sema::TDK_NonDeducedMismatch;
 
               if (ArgConstMatrix) {
                 if ((ArgConstMatrix->*GetArgDimension)() == *ParamConst)
                   return Sema::TDK_Success;
                 return Sema::TDK_NonDeducedMismatch;
               }
 
               Expr *ArgExpr = (ArgDepMatrix->*GetArgDimensionExpr)();
-              if (!ArgExpr->isValueDependent())
-                if (Optional<llvm::APSInt> ArgConst =
-                        ArgExpr->getIntegerConstantExpr(S.Context))
-                  if (*ArgConst == *ParamConst)
-                    return Sema::TDK_Success;
               return Sema::TDK_NonDeducedMismatch;
             }
 
             const NonTypeTemplateParmDecl *NTTP =
                 getDeducedParameterFromExpr(Info, ParamExpr);
             if (!NTTP)
               return Sema::TDK_Success;
 

clang/lib/Sema/SemaType.cpp

return BTy->getKind() == BuiltinType::SChar ||
          BTy->getKind() == BuiltinType::Float ||
          BTy->getKind() == BuiltinType::Half ||
          BTy->getKind() == BuiltinType::BFloat16;
 }
 
 static bool verifyValidIntegerConstantExpr(Sema &S, const ParsedAttr &Attr,
                                            llvm::APSInt &Result) {
   const auto *AttrExpr = Attr.getArgAsExpr(0);
-  if (!AttrExpr->isTypeDependent() && !AttrExpr->isValueDependent()) {
+  if (!AttrExpr->isTypeDependent()) {
     if (Optional<llvm::APSInt> Res =
             AttrExpr->getIntegerConstantExpr(S.Context)) {
       Result = *Res;
       return true;
     }
   }
   S.Diag(Attr.getLoc(), diag::err_attribute_argument_type)
       << Attr << AANT_ArgumentIntegerConstant << AttrExpr->getSourceRange();

clang/test/SemaTemplate/atomics.cpp

 
 namespace PR11320 {
   template<typename T> 
   void g(unsigned *x) { 
     __sync_bool_compare_and_swap(x, 1, 4); 
   }
   void h() { g<int>(0); }
 }
+
+// Can pass value dependent integer to atomic builtin
+template <int Order>
+void fetchAdd(int *A, int V) {
+  __atomic_fetch_add(A, V, Order);
+}
+
+void fetchAddUse(int *A, int V) {
+  fetchAdd<__ATOMIC_ACQ_REL>(A, V);
+}