[libc++] ordering comparisons should be marked __attribute__((pure))

[danakj]

https://godbolt.org/z/e4rTvMeoW

The following code generates -Wassume warnings suggesting there are side effects when converting from a strong_ordering to a bool. The same is true of partial_ordering and weak_ordering.

#include <compare>

struct S {
    __attribute__((pure)) bool g() { return true; }

    __attribute__((pure)) auto operator<=>(const S& rhs) const {
        return std::strong_ordering::equal;
    }
};


constexpr __attribute__((pure)) __attribute__((noinline)) auto f(
    const S&, const S&) noexcept {
    return std::strong_ordering::equal;
}

int main() {
    S s1, s2;
    __builtin_assume(f(s1, s2) == 0);
    __builtin_assume(f(s1, s2) == std::strong_ordering::equal);
    __builtin_assume(s1 <= s2);  // What I want to do.
    int a, b;
    __builtin_assume(a <= b);  // Only this one is accepted without warning.

    __builtin_assume(std::strong_ordering::equivalent == 0);
    __builtin_assume(std::weak_ordering::equivalent == 0);
    __builtin_assume(std::partial_ordering::equivalent == 0);
}

I tried to find the right place to insert __attribute__((pure)). I think all of these:

• llvm-project/libcxx/include/__compare/ordering.h

  Lines 157 to 213 in e3d2b58

  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator==(weak_ordering, weak_ordering) noexcept = default;
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator==(weak_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__value_ == 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator< (weak_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__value_ < 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator<=(weak_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__value_ <= 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator> (weak_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__value_ > 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator>=(weak_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__value_ >= 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator< (_CmpUnspecifiedParam, weak_ordering __v) noexcept {
  	return 0 < __v.__value_;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator<=(_CmpUnspecifiedParam, weak_ordering __v) noexcept {
  	return 0 <= __v.__value_;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator> (_CmpUnspecifiedParam, weak_ordering __v) noexcept {
  	return 0 > __v.__value_;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator>=(_CmpUnspecifiedParam, weak_ordering __v) noexcept {
  	return 0 >= __v.__value_;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr weak_ordering operator<=>(weak_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr weak_ordering operator<=>(_CmpUnspecifiedParam, weak_ordering __v) noexcept {
  	return __v < 0 ? weak_ordering::greater : (__v > 0 ? weak_ordering::less : __v);
  	}

• llvm-project/libcxx/include/__compare/ordering.h

  Lines 72 to 129 in e3d2b58

  	// comparisons
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator==(partial_ordering, partial_ordering) noexcept = default;
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator==(partial_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__is_ordered() && __v.__value_ == 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator< (partial_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__is_ordered() && __v.__value_ < 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator<=(partial_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__is_ordered() && __v.__value_ <= 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator> (partial_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__is_ordered() && __v.__value_ > 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator>=(partial_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__is_ordered() && __v.__value_ >= 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator< (_CmpUnspecifiedParam, partial_ordering __v) noexcept {
  	return __v.__is_ordered() && 0 < __v.__value_;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator<=(_CmpUnspecifiedParam, partial_ordering __v) noexcept {
  	return __v.__is_ordered() && 0 <= __v.__value_;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator> (_CmpUnspecifiedParam, partial_ordering __v) noexcept {
  	return __v.__is_ordered() && 0 > __v.__value_;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator>=(_CmpUnspecifiedParam, partial_ordering __v) noexcept {
  	return __v.__is_ordered() && 0 >= __v.__value_;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr partial_ordering operator<=>(partial_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr partial_ordering operator<=>(_CmpUnspecifiedParam, partial_ordering __v) noexcept {
  	return __v < 0 ? partial_ordering::greater : (__v > 0 ? partial_ordering::less : __v);
  	}

• llvm-project/libcxx/include/__compare/ordering.h

  Lines 249 to 305 in e3d2b58

  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator==(strong_ordering, strong_ordering) noexcept = default;
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator==(strong_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__value_ == 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator< (strong_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__value_ < 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator<=(strong_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__value_ <= 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator> (strong_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__value_ > 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator>=(strong_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v.__value_ >= 0;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator< (_CmpUnspecifiedParam, strong_ordering __v) noexcept {
  	return 0 < __v.__value_;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator<=(_CmpUnspecifiedParam, strong_ordering __v) noexcept {
  	return 0 <= __v.__value_;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator> (_CmpUnspecifiedParam, strong_ordering __v) noexcept {
  	return 0 > __v.__value_;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr bool operator>=(_CmpUnspecifiedParam, strong_ordering __v) noexcept {
  	return 0 >= __v.__value_;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr strong_ordering operator<=>(strong_ordering __v, _CmpUnspecifiedParam) noexcept {
  	return __v;
  	}
  	_LIBCPP_HIDE_FROM_ABI
  	friend constexpr strong_ordering operator<=>(_CmpUnspecifiedParam, strong_ordering __v) noexcept {
  	return __v < 0 ? strong_ordering::greater : (__v > 0 ? strong_ordering::less : __v);
  	}

[philnik777]

I don't think we want to do this. This isn't an ABI boundary, so we shouldn't have to annotate it. This would also set a precedent of annotating inline functions with [[gnu::pure]]. The next person would ask "Why isn't vector::size() annotated? I want to do __builtin_assume(vec.size() % 4 == 0)". Clang should instead either invest some more effort into checking the called functions, or move the warning to the backend, which can actually detect whether a given function has side-effects.

[danakj]

I agree clang should do this. Right now it's not possible to compare non-primitive types in __builtin_assume(). Is there some pragmatic middle ground that could allow comparisons in __builtin_assume() until Clang improves things? Is there some line we could draw between "all pure functions" and basic operations like comparison?

[philnik777]

I agree clang should do this. Right now it's not possible to compare non-primitive types in __builtin_assume(). Is there some pragmatic middle ground that could allow comparisons in __builtin_assume() until Clang improves things? Is there some line we could draw between "all pure functions" and basic operations like comparison?

I don't think there is a clear defensible line anywhere other than "we don't do it anywhere" and "we mark all the pure functions as [[gnu::pure]]". e.g. arguably std::popcount() is just as basic, and one could also see it be used in a __builtin_assume(). If you have any clear line, I'm all ears, but I don't see it.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

[pkasting]

Besides "massive effort", what would be the arguments against "[[gnu::pure/const]] everything applicable"? Unnecessary ABI break? Risk of subtle bugs in the future if one becomes non-pure/const but attribute isn't removed?

Is it possible to guesstimate the potential benefit of such a change from the optimization side? Or would fewer false-positive warnings be the primary benefit?

[pkasting]

Possible answers to my own question:

• Confusion around exception behavior: __attribute__((pure))/__attribute__((const)) is a massive unchecked footgun #58798
• Unclarity about the extent of the restrictions (e.g. is a writable function-static allowed inside a pure function): no diagnostic for gnu::pure and gnu::const functions #48110
• Implementation divergences between clang and gcc: miscompilation of calls to __attribute__((pure)) virtual functions #13130

[philnik777]

Besides "massive effort", what would be the arguments against "[[gnu::pure/const]] everything applicable"? Unnecessary ABI break? Risk of subtle bugs in the future if one becomes non-pure/const but attribute isn't removed?

Is it possible to guesstimate the potential benefit of such a change from the optimization side? Or would fewer false-positive warnings be the primary benefit?

This isn't an ABI break. My main concerns are that it will introduce very hard to find bugs without much of a benefit, as well as not having a clear line where to put these attributes. There doesn't seem to be any benefit to optimizations, since the compiler always has visibility into the implementation of the functions (we already use these attributes for functions that are in the dylib). AFAICT the only benefit to add these attributes is to avoid warnings with __builtin_assume, which makes this a very high risk undertaking with quite low rewards.

[pkasting]

In theory, the compiler having visibility into the implementation seems like it should make these provide no optimization benefit. Is that known to be true in practice? Or is it plausible these could provide meaningful hints to the compiler in any cases? Context for all these questions: was discussing trying to find code in chromium which has side effects inside the invocation of a particular macro, and someone noted that the lack of these situations on the library APIs might make it difficult to avoid false positives.

…

On Mon, Jan 29, 2024, 10:37 AM Nikolas Klauser ***@***.***> wrote: Besides "massive effort", what would be the arguments against "[[gnu::pure/const]] everything applicable"? Unnecessary ABI break? Risk of subtle bugs in the future if one becomes non-pure/const but attribute isn't removed? Is it possible to guesstimate the potential benefit of such a change from the optimization side? Or would fewer false-positive warnings be the primary benefit? This isn't an ABI break. My main concerns are that it will introduce very hard to find bugs without much of a benefit, as well as not having a clear line where to put these attributes. There doesn't seem to be any benefit to optimizations, since the compiler always has visibility into the implementation of the functions (we already use these attributes for functions that are in the dylib). AFAICT the only benefit to add these attributes is to avoid warnings with __builtin_assume, which makes this a very high risk undertaking with quite low rewards. — Reply to this email directly, view it on GitHub <#62022 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADGSAF5CQEFS3GTIVXTPZ7TYQ7T7BAVCNFSM6AAAAAAWXVLKUOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJVGMZTMNBRGI> . You are receiving this because you commented.Message ID: ***@***.***>

[philnik777]

In theory, the compiler having visibility into the implementation seems like it should make these provide no optimization benefit. Is that known to be true in practice? Or is it plausible these could provide meaningful hints to the compiler in any cases?

It's quite hard to prove a negative, but there is the function-attrs pass, which infers these attributes. The only scenario I could think of is that we are writing something, but it's OK to be discarded. I'm not aware of any function like this in libc++, so that point seems moot.

Context for all these questions: was discussing trying to find code in chromium which has side effects inside the invocation of a particular macro, and someone noted that the lack of these situations on the library APIs might make it difficult to avoid false positives.

You can easily do that by implementing your check as a pass and run the pass mentioned above before. It's not likely super pretty, but it should get the job done.