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–2083

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

[erichkeane]

clang/include/clang/AST/Stmt.h

2082–2083

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 ↗

(On Diff #344813)

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

[erichkeane]

clang/include/clang/AST/EvaluatedExprVisitor.h

37 ↗

(On Diff #344813)

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

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;
for (PartialDiagnosticAt &PDAt : It->second) {
       // Emit the note on the first diagnostic in case too many diagnostics
       // cause the note not emitted.
       if (FirstDiag && HasWarningOrError && ShowCallStack) {
         emitCallStackNotes(S, FD);
         FirstDiag = false;
       }
     }
   }
+
+  void VisitIfStmt(IfStmt *If) {
+    // Only evaluate the non-discarded side of a constexpr-if.
+    if (Optional<Stmt *> SubStmt = If->getNondiscardedCase(S.Context)) {
+      // The non-discarded statement could can be a non-existent 'else', so make
+      // sure we don't try to visit it.
+      if (*SubStmt)
+        Inherited::Visit(*SubStmt);
+    } else {
+      Inherited::VisitStmt(If);
+    }
+  }
 };
 } // namespace
 
 void Sema::emitDeferredDiags() {
   if (ExternalSource)
     ExternalSource->ReadDeclsToCheckForDeferredDiags(
         DeclsToCheckForDeferredDiags);
 

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();
 }