Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot use operator<=> in __builtin_assume #55636

Open
davidstone opened this issue May 22, 2022 · 7 comments
Open

Cannot use operator<=> in __builtin_assume #55636

davidstone opened this issue May 22, 2022 · 7 comments
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"

Comments

@davidstone
Copy link
Contributor

davidstone commented May 22, 2022

The following translation unit

#include <compare>

struct s {
	[[gnu::const]] friend auto operator<=>(s const lhs, s const rhs) {
		return std::strong_ordering::equal;
	}
};

void f(s const x) {
	__builtin_assume(x <=> x == 0);
}

causes clang to warn

<source>:10:19: warning: the argument to '__builtin_assume' has side effects that will be discarded [-Wassume]
        __builtin_assume(x <=> x == 0);
                         ^~~~~~~~~~~~
1 warning generated.
Compiler returned: 0

See it live: https://godbolt.org/z/docxWvcM6

This in turn causes __builtin_assume(x <= x); to also fail to be assumed.

I suspect this is because the comparison categories do not have their functions marked [[gnu::const]]. However, one of the functions that would need to be so marked is the helper type that's constructible from a literal 0, and it's unclear whether you are allowed to mark constructors gnu::const or gnu::pure (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51971). Furthermore, libc++ annotating its code won't help if the user is using libstdc++ or MSVC's standard library.

You can also get the same behavior with no user types defined at all:

#include <compare>

void f() {
	__builtin_assume(0 <=> 0 == 0);
}

Warns about

<source>:4:19: warning: the argument to '__builtin_assume' has side effects that will be discarded [-Wassume]
        __builtin_assume(0 <=> 0 == 0);
                         ^~~~~~~~~~~~
1 warning generated.
Compiler returned: 0

See it live: https://godbolt.org/z/EseTasTY3

@EugeneZelenko EugeneZelenko added clang:frontend Language frontend issues, e.g. anything involving "Sema" and removed new issue labels May 22, 2022
@llvmbot
Copy link
Collaborator

llvmbot commented May 22, 2022

@llvm/issue-subscribers-clang-frontend

@dscharrer
Copy link

__builtin_assume would be a lot more useful in general if it could handle function calls for inlineable functions which are not manually declared pure and if it did not throw away entire compound expressions as soon as it can't reason about any part of them.

As-is, it cannot be used with most expressions involving library types, including standard library types, which for the most part are not annotated with [[gnu::pure]] or [[gnu::const]] because doing that for functions with an inline definition is silly.

Ideally __builtin_assume(f(x)) would be the same as f(x) ? (void)0 : __builtin_unreachable() except that no code is emitted for f(x) even if the compiler can't prove that f(x) doesn't have side effects.

But then again, __builtin_assume easily crashes clang which nobody has bothered to fix for two years:

int main(int argc, char ** argv) {
  (argc == 42) ? (void)0 : __builtin_assume(1);
}

¯\_(ツ)_/¯

@3y3p4tch
Copy link

is anyone working on this currently? It seems like the referenced issue #45902 has been fixed

@cor3ntin cor3ntin added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 25, 2024
@cor3ntin
Copy link
Contributor

@ldionne Is that something you want to look at on the library side?

@3y3p4tch
Copy link

As stated by @davidstone, libc++ annotating its code won't be of much help. I believe it's a clang-frontend issue and the frontend should try harder to prove that a function contains no side-effects.

I believe the following example shows that the frontend is unable to prove constness of simple const functions.

without const (https://clang.godbolt.org/z/Ea3xfPvaq):

inline int square(int a) {
    return a*a;
}

int foo(int x) {
#ifdef __clang__
    __builtin_assume(square(x) == 16);
#else
    __attribute__((assume(square(x) == 16)));
#endif
    return square(x);
}

warns about

<source>:7:22: warning: the argument to '__builtin_assume' has side effects that will be discarded [-Wassume]
    7 |     __builtin_assume(square(x) == 16);
      |                      ^~~~~~~~~~~~~~~

with const (https://clang.godbolt.org/z/9Wsn7c3Kb):

__attribute__((const)) inline int square(int a) {
    return a*a;
}

int foo(int x) {
#ifdef __clang__
    __builtin_assume(square(x) == 16);
#else
    __attribute__((assume(square(x) == 16)));
#endif
    return square(x);
}

no warning, and clang is able to use the assumption.

@ldionne
Copy link
Member

ldionne commented Apr 25, 2024

@cor3ntin Because of the reasons stated above, I don't think it makes a lot of sense to try to bandage this from within libc++, we need a compiler solution here.

@ldionne ldionne removed the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 25, 2024
@cor3ntin
Copy link
Contributor

Clang never tries to infer pureness automatically. So calling a function without one of the pure/const attribute always has side effects
If someone wanted to investigate that, there would probably be a fair amount of work to do, notably to not negatively impact compile times and avoid false positives.
It would probably need to go through the RFC process with a solid design / prototype (and be fairly conservative)

GCC will suggest a pure attribute but AFAIK, they don't infere pureness either
https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wsuggest-attribute_003d

If the goal is to make assume more useful, the llvm optimizer is currently not capable to eliminate an expression that would have side effect - so anything that the front end does not know is side-effect-free is discarded during IR generation.
Ideally, we could have LLVM do that, in which case the optimizer could fold the whole assume expression before discarding it (and the pure attribute would not be useful), however no one is currently working on adding better assume support to the backend (afaik).

havogt pushed a commit to GridTools/gridtools that referenced this issue 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 issue 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
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
Development

No branches or pull requests

7 participants