This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mkuper on Mar 23 2015, 8:31 AM.

Details

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.

Diff Detail

Event Timeline

mkuper updated this revision to Diff 22472.Mar 23 2015, 8:31 AM
mkuper retitled this revision from to Have HasSideEffects() return false for __attribute__((const)) functions.
mkuper updated this object.
mkuper edited the test plan for this revision. (Show Details)
mkuper added reviewers: hfinkel, rsmith, aaron.ballman.
mkuper added a subscriber: Unknown Object (MLST).

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 edited edge metadata.Mar 23 2015, 9:00 AM

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

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.

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 added inline comments.Mar 23 2015, 9:19 AM
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?

I think PureAttr isn't strong enough.

char foo(char *a)
{

return *a;

}

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

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

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? :-)

rsmith edited edge metadata.Apr 5 2015, 12:02 PM

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 added inline comments.Apr 5 2015, 1:26 PM
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.

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 updated this revision to Diff 23267.Apr 6 2015, 4:28 AM
mkuper edited edge metadata.

Added handling for pure and tweaked the tests a bit.

hfinkel accepted this revision.Apr 6 2015, 4:58 AM
hfinkel edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Apr 6 2015, 4:58 AM
aaron.ballman edited edge metadata.Apr 6 2015, 5:01 AM

LGTM!

~Aaron

This revision was automatically updated to reflect the committed changes.