Have HasSideEffects() return false for __attribute__((const)) functions

[mkuper]

Summary

Currently, Expr::HasSideEffects() returns true for function-call expressions even if the function has the const attribute.
It seems like returning false in this case ought to be safe, if none of the sub-expressions have side effects.

[mkuper]

Also, there's something a bit odd about the warning text for an assume with side-effects.

"the argument to '__assume' has side effects that will be discarded" suggests that only the side effects will be discarded.
This:
(a) doesn't make a lot of sense to me, and
(b) doesn't imply that nothing at all will be emitted.
That is, for "assume(a > 0 && i++)", the warning would make me expect that what I ended up with was equivalent to "assume(a > 0)" or perhaps "assume(a > 0 && i)".

Is this just a problem with how I'm reading the warning? If not, any suggestions for alternative text?

[hfinkel]

In D8548#145202, @mkuper wrote:

Also, there's something a bit odd about the warning text for an assume with side-effects.

"the argument to '__assume' has side effects that will be discarded" suggests that only the side effects will be discarded.
This:
(a) doesn't make a lot of sense to me, and

This was done for MSVC compatibility (and to likewise avoid related questions about what the assumption should mean if it has side-effects that cannot be CSE'd with similar side effects outside the assumption).

(b) doesn't imply that nothing at all will be emitted.

Fair enough. From the user's perspective, the most "important" things is that the side effects are being dropped -- the optimizer effects are secondary. Nevertheless, we can certainly say something that the optimizer will ignore the assumption.

That is, for "assume(a > 0 && i++)", the warning would make me expect that what I ended up with was equivalent to "assume(a > 0)" or perhaps "assume(a > 0 && i)".

Reasonable point.

Is this just a problem with how I'm reading the warning? If not, any suggestions for alternative text?

How about just saying?:

the argument to '__assume' has side effects that will be discarded, and the optimizer will ignore the assumption

[mkuper]

Sorry for any confusion, Hal, I didn't mean that the implementation doesn’t make sense to me (it does!), only the phrasing of the warning.

The alternative phrasing sounds good.

[mkuper]

This is exactly the problem - that's what I'd expect from the warning text, but apparently, that is not what actually happens.
For "assume(a > 0 && i++)" the entire assumption gets thrown out.

void foo(int a, int b)
{

__builtin_assume(a > 0 && b++);

}

clang -O0 -c -S -o - -emit-llvm assume.c

assume.c:3:20: warning: the argument to '__builtin_assume' has side effects that will be discarded [-Wassume]

__builtin_assume(a > 0 && b++);
                 ^~~~~~~~~~~~

; ModuleID = 'assume.c'
target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc"

; Function Attrs: nounwind uwtable
define void @foo(i32 %a, i32 %b) #0 {
entry:

%b.addr = alloca i32, align 4
%a.addr = alloca i32, align 4
store i32 %b, i32* %b.addr, align 4
store i32 %a, i32* %a.addr, align 4
ret void

}

[hfinkel]

lib/AST/Expr.cpp

2953

You're checking for ConstAttr here, which is fine, but I think that we can do more:

This is allowed:

static int i;
void foo() {
  __builtin_assume(i > 0);
}

and so we can also check for PureAttr. And what about constexpr functions?

[mkuper]

I think PureAttr isn't strong enough.

char foo(char *a)
{

return *a;

}

is pure, but isn't side-effect free.

[hfinkel]

In D8548#145253, @mkuper wrote:

I think PureAttr isn't strong enough.

char foo(char *a)
{

return *a;

}

is pure, but isn't side-effect free.

Loads are not considered side effects currently, however. This works fine:

void bar(int *i) {
  __builtin_assume(*i > 0);
}

and, thus, my recommendation ;)

[mkuper]

Ohhh, I see, thanks, Hal!
Anyway, I'm still not sure how comfortable I feel with doing this for pure - even though it looks reasonable, and I see no rational reason why not.
I'd rather commit for const for now, and perhaps add a TODO for pure.

Does it LGTY? :-)

[mkuper]

ping

[rsmith]

Can we also do this for __attribute__((pure))? The only difference is whether the function is permitted to read globals, which is usually not a side-effect. I'm not sure whether a pure function is permitted to read volatile globals, though...

[rsmith]

lib/AST/Expr.cpp

2953

I don't think we can do anything interesting for constexpr functions, because they're not pure in general. One thing you could do is to try to evaluate the expression; if it's a constant expression, then it has no side-effects.

[mkuper]

Thanks, Richard!
Pure functions should not read volatile memory - this is explicitly called out in the gcc docs (" Interesting non-pure functions are functions with infinite loops or those depending on volatile memory"). Since both you and Hal seem certain it's fine, I'll update the patch and add that.

[mkuper]

Added handling for pure and tweaked the tests a bit.

[hfinkel]

LGTM.

[aaron.ballman]

LGTM!

~Aaron

lib/AST/Expr.cpp

bool Expr::HasSideEffects(const ASTContext &Ctx,
   case OpaqueValueExprClass:
     // These never have a side-effect.
     return false;
 
   case CallExprClass:
   case CXXOperatorCallExprClass:
   case CXXMemberCallExprClass:
   case CUDAKernelCallExprClass:
-  case BlockExprClass:
-  case CXXBindTemporaryExprClass:
-  case UserDefinedLiteralClass:
+  case UserDefinedLiteralClass: {
     // We don't know a call definitely has side effects, but we can check the
     // call's operands.
+    const Decl *FD = cast<CallExpr>(this)->getCalleeDecl();
+    bool IsConst = FD && FD->hasAttr<ConstAttr>();
+    if (IsConst || !IncludePossibleEffects)
+      break;
+    return true;
+  }
+
+  case BlockExprClass:
+  case CXXBindTemporaryExprClass:
     if (!IncludePossibleEffects)
       break;
     return true;
 
   case MSPropertyRefExprClass:
   case CompoundAssignOperatorClass:
   case VAArgExprClass:
   case AtomicExprClass:

test/CodeGen/builtin-assume.c

 // RUN: %clang_cc1 -triple x86_64-unknown-unknown -emit-llvm -o - %s | FileCheck %s
 // RUN: %clang_cc1 -triple i386-mingw32 -fms-extensions -emit-llvm -o - %s | FileCheck %s
 
+int nonconst(void);
+int isconst(void) __attribute__((const));
+
 // CHECK-LABEL: @test1
 int test1(int *a, int i) {
 // CHECK: store i32* %a, i32** [[A_ADDR:%.+]], align
 // CHECK: [[A:%.+]] = load i32*, i32** [[A_ADDR]]
 // CHECK: [[CMP:%.+]] = icmp ne i32* [[A]], null
 // CHECK: call void @llvm.assume(i1 [[CMP]])
+
+// CHECK: [[CALL:%.+]] = call i32 @isconst()
+// CHECK: [[BOOL:%.+]] = icmp ne i32 [[CALL]], 0
+// CHECK: call void @llvm.assume(i1 [[BOOL]])
+
 #ifdef _MSC_VER
   __assume(a != 0)
+  __assume(isconst());
 #else
   __builtin_assume(a != 0);
+  __builtin_assume(isconst());
 #endif
 
 // Nothing is generated for an assume with side effects...
 // CHECK-NOT: load i32*, i32** %i.addr
 // CHECK-NOT: call void @llvm.assume
+// CHECK-NOT: call i32 @nonconst()
 #ifdef _MSC_VER
   __assume(++i != 0)
+  __assume(nonconst());
 #else
   __builtin_assume(++i != 0);
+  __builtin_assume(nonconst());
 #endif
 
   return a[0];
 }
 

test/Sema/builtin-assume.c

 // RUN: %clang_cc1 -triple i386-mingw32 -fms-extensions -fsyntax-only -verify %s
 // RUN: %clang_cc1 -triple x86_64-unknown-unknown -fsyntax-only -verify %s
 
+int nonconst(void);
+int isconst(void) __attribute__((const));
+
 int foo(int *a, int i) {
 #ifdef _MSC_VER
   __assume(i != 4);
   __assume(++i > 2); //expected-warning {{the argument to '__assume' has side effects that will be discarded}}
+  __assume(nonconst() > 2); //expected-warning {{the argument to '__assume' has side effects that will be discarded}}
+  __assume(isconst() > 2);
 
   int test = sizeof(struct{char qq[(__assume(i != 5), 7)];});
 #else
   __builtin_assume(i != 4);
   __builtin_assume(++i > 2); //expected-warning {{the argument to '__builtin_assume' has side effects that will be discarded}}
+  __builtin_assume(nonconst() > 2); //expected-warning {{the argument to '__builtin_assume' has side effects that will be discarded}}
+  __builtin_assume(isconst() > 2);
 
   int test = sizeof(struct{char qq[(__builtin_assume(i != 5), 7)];});
 #endif
   return a[i];
 }