[Clang] [NFC] Clarify assume diagnostic #93077
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sirraide
added
clang:frontend
|
@llvm/pr-subscribers-clang Author: None (Sirraide) ChangesCurrently, if the argument to The issue here is that this diagnostic misrepresents what is actually happening: not only do we discard the side-effects of the expression, but we also don’t even emit any assumption information at all because the backend is not equipped to deal with eliminating side-effects in cases such as this. This has caused some confusion (see #91612) beacuse the current wording of the warning suggests that, sensibly, only the side-effects of the expression, and not the assumption itself, will be discarded. This pr updates the diagnostic to state what is actually happening: that the assumption has no effect at all because its argument contains side-effects: I’ve deliberately included ‘(potential)’ here because even expressions that only contain potential side-effects (e.g. This is intended to be temporary; we should revert back to the original diagnostic once we have proper support for assumptions with side-effects in the backend (in which case the side-effects will still be discarded, but the assumption won’t) This fixes #91612. Full diff: https://github.com/llvm/llvm-project/pull/93077.diff 5 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 41a9745ddb570..e440e0a9c8507 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -855,7 +855,7 @@ def note_strncat_wrong_size : Note<
"the terminating null byte">;
def warn_assume_side_effects : Warning<
- "the argument to %0 has side effects that will be discarded">,
+ "assumption is ignored because it contains (potential) side-effects">,
InGroup<DiagGroup<"assume">>;
def warn_omp_assume_attribute_string_unknown : Warning<
"unknown assumption string '%0'; attribute is potentially ignored">,
diff --git a/clang/test/Parser/MicrosoftExtensions.cpp b/clang/test/Parser/MicrosoftExtensions.cpp
index 6bf802a29ace3..9102bca8f6bb2 100644
--- a/clang/test/Parser/MicrosoftExtensions.cpp
+++ b/clang/test/Parser/MicrosoftExtensions.cpp
@@ -426,7 +426,7 @@ bool f(int);
template <typename T>
struct A {
constexpr A(T t) {
- __assume(f(t)); // expected-warning{{the argument to '__assume' has side effects that will be discarded}}
+ __assume(f(t)); // expected-warning{{assumption is ignored because it contains (potential) side-effects}}
}
constexpr bool g() { return false; }
};
diff --git a/clang/test/Sema/builtin-assume.c b/clang/test/Sema/builtin-assume.c
index 932fb5c973eb0..21d62d8fd06c1 100644
--- a/clang/test/Sema/builtin-assume.c
+++ b/clang/test/Sema/builtin-assume.c
@@ -8,20 +8,20 @@ int ispure(int) __attribute__((pure));
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(++i > 2); //expected-warning {{assumption is ignored because it contains (potential) side-effects}}
+ __assume(nonconst() > 2); //expected-warning {{assumption is ignored because it contains (potential) side-effects}}
__assume(isconst() > 2);
__assume(ispure(i) > 2);
- __assume(ispure(++i) > 2); //expected-warning {{the argument to '__assume' has side effects that will be discarded}}
+ __assume(ispure(++i) > 2); //expected-warning {{assumption is ignored because it contains (potential) side-effects}}
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(++i > 2); //expected-warning {{assumption is ignored because it contains (potential) side-effects}}
+ __builtin_assume(nonconst() > 2); //expected-warning {{assumption is ignored because it contains (potential) side-effects}}
__builtin_assume(isconst() > 2);
__builtin_assume(ispure(i) > 2);
- __builtin_assume(ispure(++i) > 2); //expected-warning {{the argument to '__builtin_assume' has side effects that will be discarded}}
+ __builtin_assume(ispure(++i) > 2); //expected-warning {{assumption is ignored because it contains (potential) side-effects}}
int test = sizeof(struct{char qq[(__builtin_assume(i != 5), 7)];}); // expected-warning {{variable length array}}
#endif
diff --git a/clang/test/Sema/stmtexprs.c b/clang/test/Sema/stmtexprs.c
index 708fc9abb75c0..7493bbcef363d 100644
--- a/clang/test/Sema/stmtexprs.c
+++ b/clang/test/Sema/stmtexprs.c
@@ -4,6 +4,6 @@ int stmtexpr_fn(void);
void stmtexprs(int i) {
__builtin_assume( ({ 1; }) ); // no warning about "side effects"
__builtin_assume( ({ if (i) { (void)0; }; 42; }) ); // no warning about "side effects"
- // expected-warning@+1 {{the argument to '__builtin_assume' has side effects that will be discarded}}
+ // expected-warning@+1 {{assumption is ignored because it contains (potential) side-effects}}
__builtin_assume( ({ if (i) ({ stmtexpr_fn(); }); 1; }) );
}
diff --git a/clang/test/SemaCXX/cxx23-assume.cpp b/clang/test/SemaCXX/cxx23-assume.cpp
index ea71e7b251823..9138501d726dd 100644
--- a/clang/test/SemaCXX/cxx23-assume.cpp
+++ b/clang/test/SemaCXX/cxx23-assume.cpp
@@ -28,7 +28,7 @@ bool f2();
template <typename T>
constexpr void f3() {
- [[assume(T{})]]; // expected-error {{not contextually convertible to 'bool'}} expected-warning {{has side effects that will be discarded}} ext-warning {{C++23 extension}}
+ [[assume(T{})]]; // expected-error {{not contextually convertible to 'bool'}} expected-warning {{assumption is ignored because it contains (potential) side-effects}} ext-warning {{C++23 extension}}
}
void g(int x) {
@@ -38,13 +38,13 @@ void g(int x) {
S<false>{}.f();
S<true>{}.g<char>();
S<true>{}.g<int>();
- [[assume(f2())]]; // expected-warning {{side effects that will be discarded}} ext-warning {{C++23 extension}}
+ [[assume(f2())]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} ext-warning {{C++23 extension}}
- [[assume((x = 3))]]; // expected-warning {{has side effects that will be discarded}} // ext-warning {{C++23 extension}}
- [[assume(x++)]]; // expected-warning {{has side effects that will be discarded}} // ext-warning {{C++23 extension}}
- [[assume(++x)]]; // expected-warning {{has side effects that will be discarded}} // ext-warning {{C++23 extension}}
- [[assume([]{ return true; }())]]; // expected-warning {{has side effects that will be discarded}} // ext-warning {{C++23 extension}}
- [[assume(B{})]]; // expected-warning {{has side effects that will be discarded}} // ext-warning {{C++23 extension}}
+ [[assume((x = 3))]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} // ext-warning {{C++23 extension}}
+ [[assume(x++)]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} // ext-warning {{C++23 extension}}
+ [[assume(++x)]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} // ext-warning {{C++23 extension}}
+ [[assume([]{ return true; }())]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} // ext-warning {{C++23 extension}}
+ [[assume(B{})]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} // ext-warning {{C++23 extension}}
[[assume((1, 2))]]; // expected-warning {{has no effect}} // ext-warning {{C++23 extension}}
f3<A>(); // expected-note {{in instantiation of}}
@@ -91,7 +91,7 @@ static_assert(S<false>{}.g<A>()); // expected-error {{not an integral constant e
template <typename T>
constexpr bool f4() {
- [[assume(!T{})]]; // expected-error {{invalid argument type 'D'}} // expected-warning 2 {{side effects}} ext-warning {{C++23 extension}}
+ [[assume(!T{})]]; // expected-error {{invalid argument type 'D'}} // expected-warning 2 {{assumption is ignored because it contains (potential) side-effects}} ext-warning {{C++23 extension}}
return sizeof(T) == sizeof(int);
}
@@ -137,8 +137,8 @@ static_assert(f5<F>() == 2); // expected-note {{while checking constraint satisf
// Do not validate assumptions whose evaluation would have side-effects.
constexpr int foo() {
int a = 0;
- [[assume(a++)]] [[assume(++a)]]; // expected-warning 2 {{has side effects that will be discarded}} ext-warning 2 {{C++23 extension}}
- [[assume((a+=1))]]; // expected-warning {{has side effects that will be discarded}} ext-warning {{C++23 extension}}
+ [[assume(a++)]] [[assume(++a)]]; // expected-warning 2 {{assumption is ignored because it contains (potential) side-effects}} ext-warning 2 {{C++23 extension}}
+ [[assume((a+=1))]]; // expected-warning {{assumption is ignored because it contains (potential) side-effects}} ext-warning {{C++23 extension}}
return a;
}
@@ -154,7 +154,7 @@ int
foo (int x, int y)
{
__attribute__((assume(x == 42)));
- __attribute__((assume(++y == 43))); // expected-warning {{has side effects that will be discarded}}
+ __attribute__((assume(++y == 43))); // expected-warning {{assumption is ignored because it contains (potential) side-effects}}
return x + y;
}
}
|
|
Thanks, this is very good |
havogt
pushed a commit
to GridTools/gridtools
that referenced
this pull request
Jun 24, 2024
Building the project with `Clang` generates the following warning: ``` the argument to '__builtin_assume' has side effects that will be discarded [-Wassume] ``` Seems like Clang doesn't handle the hint well unless we pass a const/pure attribute to the function call inside the assume. See: llvm/llvm-project#55636 and llvm/llvm-project#93077 With this change performance for the fn fused nabla examples is significantly improved on clang-cuda and is now much faster than (our old) nvcc, which doesn't have `assume` support.
havogt
pushed a commit
to GridTools/gridtools
that referenced
this pull request
Jun 26, 2024
Building the project with `Clang` generates the following warning: ``` the argument to '__builtin_assume' has side effects that will be discarded [-Wassume] ``` Seems like Clang doesn't handle the hint well unless we pass a const/pure attribute to the function call inside the assume. See: llvm/llvm-project#55636 and llvm/llvm-project#93077 With this change performance for the fn fused nabla examples is significantly improved on clang-cuda and is now much faster than (our old) nvcc, which doesn't have `assume` support.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, if the argument to
__builtin_assumeand friends contains side-effects, we issue the following diagnostic:The issue here is that this diagnostic misrepresents what is actually happening: not only do we discard the side-effects of the expression, but we also don’t even emit any assumption information at all because the backend is not equipped to deal with eliminating side-effects in cases such as this.
This has caused some confusion (see #91612) beacuse the current wording of the warning suggests that, sensibly, only the side-effects of the expression, and not the assumption itself, will be discarded.
This pr updates the diagnostic to state what is actually happening: that the assumption has no effect at all because its argument contains side-effects:
I’ve deliberately included ‘(potential)’ here because even expressions that only contain potential side-effects (e.g.
true ? x : x++or a call to a function that is pure, but we don’t know that it is) cause the assumption to be discarded. This, too, has caused some confusion because it was erroneously assumed that Clang would e.g. infer that a function call is pure and not discard the assumption as a result when that isn’t the case.This is intended to be temporary; we should revert back to the original diagnostic once we have proper support for assumptions with side-effects in the backend (in which case the side-effects will still be discarded, but the assumption won’t)
This fixes #91612.