Suppress Deferred Diagnostics in discarded statements.

[erichkeane]

Summary

It doesn't really make sense to emit language specific diagnostics
in a discarded statement, and suppressing these diagnostics results in a
programming pattern that many users will feel is quite useful.

Basically, this makes sure we only emit errors from the 'true' side of a
'constexpr if'.

[ABataev]

clang/include/clang/AST/Stmt.h

2082–2084

Do you really need Optional here? Just nullptr is not enough?

[erichkeane]

clang/include/clang/AST/Stmt.h

2082–2084

Yes for 2 reasons:
1- This is the existing interface, I didn't see reason to change it.

2- Optional::None and nullptr are two different 'cases'. None is returned here when this is not a constexpr-if. Nullptr is returned in cases where it _IS_ a constexpr-if, but the nondiscarded case is not present. For example:

if (foo) {} // Returns None.

if constexpr (false) {} // Returns nullptr.

if constexpr (false) else {} // Returns the 'else' compound statement.

[ABataev]

LG

[erichkeane]

In D102251#2751101, @ABataev wrote:

LG

Thanks! I'm going to give @yaxunl a day to take a look that this, particularly since the only test I could come up with was CUDA based.

[ABataev]

In D102251#2751226, @erichkeane wrote:

In D102251#2751101, @ABataev wrote:

LG

Thanks! I'm going to give @yaxunl a day to take a look that this, particularly since the only test I could come up with was CUDA based.

Yep!

[tra]

LGTM for CUDA. This matches the intent of deferred diags -- we only emit them if we get to generate the code for the sources that triggered them, so they should not show up for the false constexpr branches.

[yaxunl]

LGTM. Thanks.

[rsmith]

(No blocking concerns here.)

clang/lib/AST/Stmt.cpp

1000–1003

Can this simply be

return const_cast<IfStmt *>(this)->getNondiscardedCase(Ctx));

? I would expect that if T implicitly converts to U then Optional<T> implicitly converts to Optional<U>.

clang/lib/Sema/Sema.cpp

1545

I wonder if there are other UsedDeclVisitors that would want this behavior, largely because of [basic.def.odr]p10: "Every program shall contain exactly one definition of every non-inline function or variable that is odr-used in that program outside of a discarded statement (8.5.2)". It seems worth checking, and maybe moving this functionality to an optional behavior in the base class, but I don't want to hold up this patch on that.

[erichkeane]

clang/lib/AST/Stmt.cpp

1000–1003

that does not seem to be the case unfortunately. llvm::Optional doesn't have any such conversion functions. Its only constructors and or operator= just take Optional. It is missing most of the constructors here: https://en.cppreference.com/w/cpp/utility/optional/optional

clang/lib/Sema/Sema.cpp

1545

I've actually implemented this 2x (another time in a different class, that inherits from a different visitor, I believe CallGraph) in our downstream. The issue in many cases is that we lack access to ASTContext in those.

HOWEVER, I note that EvaluatedExprVisitorBase actually has access to an ASTContext, so I can put that in there controlled by an inherited function (like we do with some of the other visitors).

If it ends up being low-touch enough, I'll move it as an opt-in behavior for this patch this morning.

[erichkeane]

Moved IfStmt/discarded case logic to ExprEvaluatorBase per @rsmith 's suggestion. This is the 'highest' in the tree that has a Sema/ASTContext reference, so anything 'higher' in the tree would be higher touch. @rsmith: WDYT?

[rsmith]

clang/include/clang/AST/EvaluatedExprVisitor.h

37

Maybe shouldVisitDiscardedStmt would be more obvious, since "discarded statement" is the standard terminology.

[erichkeane]

clang/include/clang/AST/EvaluatedExprVisitor.h

37

Yep, agreed. I'll do that as a part of the commit.

clang/include/clang/AST/EvaluatedExprVisitor.h

 /// Given a potentially-evaluated expression, this visitor visits all
 /// of its potentially-evaluated subexpressions, recursively.
 template<template <typename> class Ptr, typename ImplClass>
 class EvaluatedExprVisitorBase : public StmtVisitorBase<Ptr, ImplClass, void> {
 protected:
   const ASTContext &Context;
 
 public:
+  // Return whether this visitor should recurse into discarded statements for a
+  // 'constexpr-if'.
+  bool shouldVisitDiscardedStmt() const { return true; }
 #define PTR(CLASS) typename Ptr<CLASS>::type
 
   explicit EvaluatedExprVisitorBase(const ASTContext &Context) : Context(Context) { }
 
   // Expressions that have no potentially-evaluated subexpressions (but may have
   // other sub-expressions).
   void VisitDeclRefExpr(PTR(DeclRefExpr) E) { }
   void VisitOffsetOfExpr(PTR(OffsetOfExpr) E) { }
#define PTR(CLASS) typename Ptr<CLASS>::type
 
   void VisitCXXTypeidExpr(PTR(CXXTypeidExpr) E) {
     if (E->isPotentiallyEvaluated())
       return this->Visit(E->getExprOperand());
   }
 
   void VisitCallExpr(PTR(CallExpr) CE) {
     if (!CE->isUnevaluatedBuiltinCall(Context))
-      return static_cast<ImplClass*>(this)->VisitExpr(CE);
+      return getDerived().VisitExpr(CE);
   }
 
   void VisitLambdaExpr(PTR(LambdaExpr) LE) {
     // Only visit the capture initializers, and not the body.
     for (LambdaExpr::const_capture_init_iterator I = LE->capture_init_begin(),
                                                  E = LE->capture_init_end();
          I != E; ++I)
       if (*I)
         this->Visit(*I);
   }
 
   /// The basis case walks all of the children of the statement or
   /// expression, assuming they are all potentially evaluated.
   void VisitStmt(PTR(Stmt) S) {
     for (auto *SubStmt : S->children())
       if (SubStmt)
         this->Visit(SubStmt);
   }
 
+  void VisitIfStmt(PTR(IfStmt) If) {
+    if (!getDerived().shouldVisitDiscardedStmt()) {
+      if (auto SubStmt = If->getNondiscardedCase(Context)) {
+        if (*SubStmt)
+          this->Visit(*SubStmt);
+        return;
+      }
+    }
+
+    getDerived().VisitStmt(If);
+  }
+
+  ImplClass &getDerived() { return *static_cast<ImplClass *>(this); }
+
 #undef PTR
 };
 
 /// EvaluatedExprVisitor - This class visits 'Expr *'s
 template <typename ImplClass>
 class EvaluatedExprVisitor
     : public EvaluatedExprVisitorBase<std::add_pointer, ImplClass> {
 public:

clang/include/clang/AST/Stmt.h

void setElseLoc(SourceLocation ElseLoc) {
     *getTrailingObjects<SourceLocation>() = ElseLoc;
   }
 
   bool isConstexpr() const { return IfStmtBits.IsConstexpr; }
   void setConstexpr(bool C) { IfStmtBits.IsConstexpr = C; }
 
   /// If this is an 'if constexpr', determine which substatement will be taken.
   /// Otherwise, or if the condition is value-dependent, returns None.
   Optional<const Stmt*> getNondiscardedCase(const ASTContext &Ctx) const;
+  Optional<Stmt *> getNondiscardedCase(const ASTContext &Ctx);
 
   bool isObjCAvailabilityCheck() const;
 
   SourceLocation getBeginLoc() const { return getIfLoc(); }
   SourceLocation getEndLoc() const LLVM_READONLY {
     if (getElse())
       return getElse()->getEndLoc();
     return getThen()->getEndLoc();
   }

clang/lib/AST/Stmt.cpp

void IfStmt::setConditionVariable(const ASTContext &Ctx, VarDecl *V) {
   getTrailingObjects<Stmt *>()[varOffset()] = new (Ctx)
       DeclStmt(DeclGroupRef(V), VarRange.getBegin(), VarRange.getEnd());
 }
 
 bool IfStmt::isObjCAvailabilityCheck() const {
   return isa<ObjCAvailabilityCheckExpr>(getCond());
 }
 
-Optional<const Stmt*> IfStmt::getNondiscardedCase(const ASTContext &Ctx) const {
+Optional<Stmt *> IfStmt::getNondiscardedCase(const ASTContext &Ctx) {
   if (!isConstexpr() || getCond()->isValueDependent())
     return None;
   return !getCond()->EvaluateKnownConstInt(Ctx) ? getElse() : getThen();
 }
 
+Optional<const Stmt *>
+IfStmt::getNondiscardedCase(const ASTContext &Ctx) const {
+  if (Optional<Stmt *> Result =
+          const_cast<IfStmt *>(this)->getNondiscardedCase(Ctx))
+    return *Result;
+  return None;
+}
+
 ForStmt::ForStmt(const ASTContext &C, Stmt *Init, Expr *Cond, VarDecl *condVar,
                  Expr *Inc, Stmt *Body, SourceLocation FL, SourceLocation LP,
                  SourceLocation RP)
   : Stmt(ForStmtClass), LParenLoc(LP), RParenLoc(RP)
 {
   SubExprs[INIT] = Init;
   setConditionVariable(C, condVar);
   SubExprs[COND] = Cond;

clang/lib/Sema/Sema.cpp

 /// the tree nodes traversed from the tree root to the current node form a use
 /// stack. The emission state of the current node depends on two factors:
 ///    1. the emission state of the root node
 ///    2. whether the current node is in OpenMP device context
 /// If the function is decided to be emitted, its contained deferred diagnostics
 /// are emitted, together with the information about the use stack.
 ///
 class DeferredDiagnosticsEmitter
     : public UsedDeclVisitor<DeferredDiagnosticsEmitter> {
 public:
   typedef UsedDeclVisitor<DeferredDiagnosticsEmitter> Inherited;
 
   // Whether the function is already in the current use-path.
   llvm::SmallPtrSet<CanonicalDeclPtr<Decl>, 4> InUsePath;
 
   // The current use-path.
   llvm::SmallVector<CanonicalDeclPtr<FunctionDecl>, 4> UsePath;
public:
   // Current OpenMP device context level. It is initialized to 0 and each
   // entering of device context increases it by 1 and each exit decreases
   // it by 1. Non-zero value indicates it is currently in device context.
   unsigned InOMPDeviceContext;
 
   DeferredDiagnosticsEmitter(Sema &S)
       : Inherited(S), ShouldEmitRootNode(false), InOMPDeviceContext(0) {}
 
+  bool shouldVisitDiscardedStmt() const { return false; }
+
   void VisitOMPTargetDirective(OMPTargetDirective *Node) {
     ++InOMPDeviceContext;
     Inherited::VisitOMPTargetDirective(Node);
     --InOMPDeviceContext;
   }
 
   void visitUsedDecl(SourceLocation Loc, Decl *D) {
     if (isa<VarDecl>(D))

clang/test/SemaCUDA/deferred-diags.cu

-// RUN: %clang_cc1 -fcxx-exceptions -fcuda-is-device -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fcxx-exceptions -fcuda-is-device -fsyntax-only -std=c++17 -verify %s
 
 #include "Inputs/cuda.h"
 
 // Error, instantiated on device.
 inline __host__ __device__ void hasInvalid() {
   throw NULL;
   // expected-error@-1 2{{cannot use 'throw' in __host__ __device__ function}}
 }
 
+inline __host__ __device__ void hasInvalid2() {
+  throw NULL;
+  // expected-error@-1 2{{cannot use 'throw' in __host__ __device__ function}}
+}
+
+inline __host__ __device__ void hasInvalidDiscarded() {
+  // This is only used in the discarded statements below, so this should not diagnose.
+  throw NULL;
+}
+
 static __device__ void use0() {
   hasInvalid(); // expected-note {{called by 'use0'}}
   hasInvalid(); // expected-note {{called by 'use0'}}
+
+  if constexpr (true) {
+    hasInvalid2(); // expected-note {{called by 'use0'}}
+  } else {
+    hasInvalidDiscarded();
+  }
+
+  if constexpr (false) {
+    hasInvalidDiscarded();
+  } else {
+    hasInvalid2(); // expected-note {{called by 'use0'}}
+  }
+
+  if constexpr (false) {
+    hasInvalidDiscarded();
+  }
 }
 
 // To avoid excessive diagnostic messages, deferred diagnostics are only
 // emitted the first time a function is called.
 static __device__ void use1() {
-  use0(); // expected-note 2{{called by 'use1'}}
+  use0(); // expected-note 4{{called by 'use1'}}
   use0();
 }
 
 static __device__ void use2() {
-  use1(); // expected-note 2{{called by 'use2'}}
+  use1(); // expected-note 4{{called by 'use2'}}
   use1();
 }
 
 static __device__ void use3() {
-  use2(); // expected-note 2{{called by 'use3'}}
+  use2(); // expected-note 4{{called by 'use3'}}
   use2();
 }
 
 __global__ void use4() {
-  use3(); // expected-note 2{{called by 'use4'}}
+  use3(); // expected-note 4{{called by 'use4'}}
   use3();
 }