llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.81k stars 11.91k forks source link

Cannot use `operator<=>` in `__builtin_assume` #55636

Open davidstone opened 2 years ago

davidstone commented 2 years ago

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

llvmbot commented 2 years ago

@llvm/issue-subscribers-clang-frontend

dscharrer commented 2 years ago

__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 commented 6 months ago

is anyone working on this currently? It seems like the referenced issue https://github.com/llvm/llvm-project/issues/45902 has been fixed

cor3ntin commented 6 months ago

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

3y3p4tch commented 6 months ago

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 commented 6 months ago

@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.

cor3ntin commented 6 months ago

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