Update documentation of HasSideEffects to match its callers' expectations, and

update implementation to match. An elidable, non-trivial constructor call is a
side-effect under this definition, but wasn't under the old one, because we are
not required to evaluate it even though it may have an effect.

Also rationalize checking for volatile reads: just look for lvalue-to-rvalue
conversions on volatile glvalues, and ignore whether a DeclRefExpr etc is for
a volatile variable.


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@161393 91177308-0d34-0410-b5e6-96231b3b80d8

3 files changed, 35 insertions, 26 deletions

diff --git a/include/clang/AST/Expr.h b/include/clang/AST/Expr.h
index bb66e710ae..29e3649a27 100644
--- a/include/clang/AST/Expr.h
+++ b/include/clang/AST/Expr.h
@@ -510,9 +510,8 @@ public:
   bool isEvaluatable(const ASTContext &Ctx) const;
 
   /// HasSideEffects - This routine returns true for all those expressions
-  /// which must be evaluated each time and must not be optimized away
-  /// or evaluated at compile time. Example is a function call, volatile
-  /// variable read.
+  /// which have any effect other than producing a value. Example is a function
+  /// call, volatile variable read, or throwing an exception.
   bool HasSideEffects(const ASTContext &Ctx) const;
 
   /// \brief Determine whether this expression involves a call to any function
diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp
index faba404b9a..9df850d6bd 100644
--- a/lib/AST/Expr.cpp
+++ b/lib/AST/Expr.cpp
@@ -2643,6 +2643,8 @@ bool Expr::HasSideEffects(const ASTContext &Ctx) const {
   case SubstNonTypeTemplateParmPackExprClass:
     llvm_unreachable("shouldn't see dependent / unresolved nodes here");
 
+  case DeclRefExprClass:
+  case ObjCIvarRefExprClass:
   case PredefinedExprClass:
   case IntegerLiteralClass:
   case FloatingLiteralClass:
@@ -2696,16 +2698,10 @@ bool Expr::HasSideEffects(const ASTContext &Ctx) const {
   case MemberExprClass:
   case ConditionalOperatorClass:
   case BinaryConditionalOperatorClass:
-  case ImplicitCastExprClass:
-  case CStyleCastExprClass:
   case CompoundLiteralExprClass:
   case ExtVectorElementExprClass:
   case DesignatedInitExprClass:
   case ParenListExprClass:
-  case CXXStaticCastExprClass:
-  case CXXReinterpretCastExprClass:
-  case CXXConstCastExprClass:
-  case CXXFunctionalCastExprClass:
   case CXXPseudoDestructorExprClass:
   case SubstNonTypeTemplateParmExprClass:
   case MaterializeTemporaryExprClass:
@@ -2714,24 +2710,16 @@ bool Expr::HasSideEffects(const ASTContext &Ctx) const {
     // These have a side-effect if any subexpression does.
     break;
 
-  case UnaryOperatorClass: {
-    const UnaryOperator *UO = cast<UnaryOperator>(this);
-    if (UO->isIncrementDecrementOp())
-      return true;
-    if (UO->getOpcode() == UO_Deref && UO->getType().isVolatileQualified())
+  case UnaryOperatorClass:
+    if (cast<UnaryOperator>(this)->isIncrementDecrementOp())
       return true;
     break;
-  }
 
   case BinaryOperatorClass:
     if (cast<BinaryOperator>(this)->isAssignmentOp())
       return true;
     break;
 
-  case DeclRefExprClass:
-  case ObjCIvarRefExprClass:
-    return getType().isVolatileQualified();
-
   case InitListExprClass:
     // FIXME: The children for an InitListExpr doesn't include the array filler.
     if (const Expr *E = cast<InitListExpr>(this)->getArrayFiller())
@@ -2755,7 +2743,17 @@ bool Expr::HasSideEffects(const ASTContext &Ctx) const {
     if (DCE->getTypeAsWritten()->isReferenceType() &&
         DCE->getCastKind() == CK_Dynamic)
       return true;
-    // Also has side-effects if the subexpression does.
+  } // Fall through.
+  case ImplicitCastExprClass:
+  case CStyleCastExprClass:
+  case CXXStaticCastExprClass:
+  case CXXReinterpretCastExprClass:
+  case CXXConstCastExprClass:
+  case CXXFunctionalCastExprClass: {
+    const CastExpr *CE = cast<CastExpr>(this);
+    if (CE->getCastKind() == CK_LValueToRValue &&
+        CE->getSubExpr()->getType().isVolatileQualified())
+      return true;
     break;
   }
 
@@ -2779,10 +2777,10 @@ bool Expr::HasSideEffects(const ASTContext &Ctx) const {
   case CXXConstructExprClass:
   case CXXTemporaryObjectExprClass: {
     const CXXConstructExpr *CE = cast<CXXConstructExpr>(this);
-    if (!CE->isElidable() && !CE->getConstructor()->isTrivial())
+    if (!CE->getConstructor()->isTrivial())
       return true;
-    // An elidable or trivial constructor does not add any side-effects of its
-    // own. Just look at its arguments.
+    // A trivial constructor does not add any side-effects of its own. Just look
+    // at its arguments.
     break;
   }
 
diff --git a/test/CodeGenCXX/global-init.cpp b/test/CodeGenCXX/global-init.cpp
index da5467ac3f..2a53ad9e3e 100644
--- a/test/CodeGenCXX/global-init.cpp
+++ b/test/CodeGenCXX/global-init.cpp
@@ -129,6 +129,14 @@ namespace test6 {
   const std::type_info *const t = &typeid(p);
   // CHECK-NOT: @_ZN5test6L1tE
 
+  extern B *volatile v;
+  // CHECK: store {{.*}} @_ZN5test6L1wE
+  B *const w = dynamic_cast<B*>(v);
+
+  // CHECK: load volatile
+  // CHECK: store {{.*}} @_ZN5test6L1xE
+  const int x = *(volatile int*)0x1234;
+
   namespace {
     int a = int();
     volatile int b = int();
@@ -145,7 +153,7 @@ namespace test6 {
 namespace test7 {
   struct A { A(); };
   struct B { ~B(); int n; };
-  struct C { C() = default; C(const C&); };
+  struct C { C() = default; C(const C&); int n; };
   struct D {};
 
   // CHECK: call void @_ZN5test71AC1Ev({{.*}}@_ZN5test7L1aE)
@@ -161,8 +169,12 @@ namespace test7 {
 
   // CHECK-NOT: @_ZN5test7L2c1E
   // CHECK: @_ZN5test7L2c2E
-  const C c1 = C(); // elidable copy
-  const C c2 = static_cast<C&&>(C()); // non-elidable copy
+  // CHECK-NOT: @_ZN5test7L2c3E
+  // CHECK: @_ZN5test7L2c4E
+  const C c1 = C();
+  const C c2 = static_cast<const C&>(C());
+  const int c3 = C().n;
+  const int c4 = C(C()).n;
 
   // CHECK-NOT: @_ZN5test7L1dE
   const D d = D();