__builtin_assume with embedded function calls are ignored

[davidben]

I'm not sure if this is a bug or working as intended, but it surprised me. Consider this code:

bool f1(std::string_view sv, size_t n) {
    return n <= sv.size();
}

bool f2(std::string_view sv, size_t n) {
    __builtin_assume(n <= sv.size());
    return n <= sv.size();
}

bool f3(std::string_view sv, size_t n) {
    size_t size = sv.size();
    __builtin_assume(n <= size);
    return n <= sv.size();
}

https://godbolt.org/z/1G6cao4ss

While this is a toy function, this is a minimized version of plausible patterns in real world code that wishes to simultaneously use bounds-checked STL mode, but also needs to manually annotate a few performance-critical invariants that the compiler is unable to discover.

Although f2 and f3 look like they would be the same, Clang only deletes the check in f3. Now, Clang does emit a warning, so perhaps this is intended, but the warning says:

<source>:8:22: warning: the argument to '__builtin_assume' has side effects that will be discarded [-Wassume]
    8 |     __builtin_assume(n <= sv.size());
      |    

This reads to me like saying that the side effects in sv.size() will be discarded at program runtime, but not that the compiler will also discard the stated invariant. As a programmer, I would read this and think, well, okay, sv.size() actually no side effects but the type system doesn't know that, so I can silence that warning, and assume that the optimizer will still figure it out.

However, it seems that it actually means that the whole invariant is discarded and the optimizer doesn't use the invariant. This is a bit non-obvious. It can be worked around with the pattern in f3, but:

• The programmer needs to know to do that
• It is more verbose
• Presumably __builtin_assume will actually be a macro that expands to nothing on non-Clang compilers, but then we'll get a warning about unused size, which is extra tedious.

[danakj]

Noting here that libc++'s _LIBCPP_ASSUME macro actually just turns off the warning before calling __builtin_assume so the developer will have no diagnostic at all to know it's doing nothing (if indeed that is always the outcome).

llvm-project/libcxx/include/__assert

Lines 29 to 35 in 2083e97

	#if 0 && __has_builtin(__builtin_assume)
	# define _LIBCPP_ASSUME(expression) \
	(_LIBCPP_DIAGNOSTIC_PUSH _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wassume") \
	__builtin_assume(static_cast<bool>(expression)) _LIBCPP_DIAGNOSTIC_POP)
	#else
	# define _LIBCPP_ASSUME(expression) ((void)0)
	#endif

[davidben]

@danakj Yeah, I think we should fix this so that _LIBCPP_ASSUME still does __builtin_assume but we turn off the automatic thing where disabled assertions become assumes. (Possibly indefinitely because some assertions are off even in hardened mode, but every __builtin_assume is a safety hazard. They're sometimes useful, but probably the bar for adding them should be higher than "someone wanted a debug assert somewhere".)

[Sirraide]

Yeah, this is a known issue. To summarise: LLVM is currently not equipped to handle assumptions with side-effects properly, so we just drop the assumption if it has side-effects. The same applies to C++23’s [[assume]] attribute.

[Sirraide]

See also: https://discourse.llvm.org/t/rfc-builtin-assume-assume-optimization-behavior/76943

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: David Benjamin (davidben)

I'm not sure if this is a bug or working as intended, but it surprised me. Consider this code:

bool f1(std::string_view sv, size_t n) {
    return n <= sv.size();
}

bool f2(std::string_view sv, size_t n) {
    __builtin_assume(n <= sv.size());
    return n <= sv.size();
}

bool f3(std::string_view sv, size_t n) {
    size_t size = sv.size();
    __builtin_assume(n <= size);
    return n <= sv.size();
}

https://godbolt.org/z/1G6cao4ss

While this is a toy function, this is a minimized version of plausible patterns in real world code that wishes to simultaneously use bounds-checked STL mode, but also needs to manually annotate a few performance-critical invariants that the compiler is unable to discover.

Although f2 and f3 look like they would be the same, Clang only deletes the check in f3. Now, Clang does emit a warning, so perhaps this is intended, but the warning says:

<source>:8:22: warning: the argument to '__builtin_assume' has side effects that will be discarded [-Wassume]
    8 |     __builtin_assume(n <= sv.size());
      |    

This reads to me like saying that the side effects in sv.size() will be discarded at program runtime, but not that the compiler will also discard the stated invariant. As a programmer, I would read this and think, well, okay, sv.size() actually no side effects but the type system doesn't know that, so I can silence that warning, and assume that the optimizer will still figure it out.

However, it seems that it actually means that the whole invariant is discarded and the optimizer doesn't use the invariant. This is a bit non-obvious. It can be worked around with the pattern in f3, but:

• The programmer needs to know to do that
• It is more verbose
• Presumably __builtin_assume will actually be a macro that expands to nothing on non-Clang compilers, but then we'll get a warning about unused size, which is extra tedious.

[llvmbot]

@llvm/issue-subscribers-clang-frontend

Author: David Benjamin (davidben)

I'm not sure if this is a bug or working as intended, but it surprised me. Consider this code:

bool f1(std::string_view sv, size_t n) {
    return n <= sv.size();
}

bool f2(std::string_view sv, size_t n) {
    __builtin_assume(n <= sv.size());
    return n <= sv.size();
}

bool f3(std::string_view sv, size_t n) {
    size_t size = sv.size();
    __builtin_assume(n <= size);
    return n <= sv.size();
}

https://godbolt.org/z/1G6cao4ss

While this is a toy function, this is a minimized version of plausible patterns in real world code that wishes to simultaneously use bounds-checked STL mode, but also needs to manually annotate a few performance-critical invariants that the compiler is unable to discover.

Although f2 and f3 look like they would be the same, Clang only deletes the check in f3. Now, Clang does emit a warning, so perhaps this is intended, but the warning says:

<source>:8:22: warning: the argument to '__builtin_assume' has side effects that will be discarded [-Wassume]
    8 |     __builtin_assume(n <= sv.size());
      |    

This reads to me like saying that the side effects in sv.size() will be discarded at program runtime, but not that the compiler will also discard the stated invariant. As a programmer, I would read this and think, well, okay, sv.size() actually no side effects but the type system doesn't know that, so I can silence that warning, and assume that the optimizer will still figure it out.

However, it seems that it actually means that the whole invariant is discarded and the optimizer doesn't use the invariant. This is a bit non-obvious. It can be worked around with the pattern in f3, but:

• The programmer needs to know to do that
• It is more verbose
• Presumably __builtin_assume will actually be a macro that expands to nothing on non-Clang compilers, but then we'll get a warning about unused size, which is extra tedious.

[Sirraide]

I think we should fix this so that _LIBCPP_ASSUME still does __builtin_assume

Also, as a remark, iirc someone on the libc++ team disabled _LIBCPP_ASSUME beacuse __builtin_assume can currently actually worsen codegen in some cases, so it currently should not be used w/o actually testing that it improves something.

[davidben]

That discussion seems unrelated to this issue. That seems to talk about how LLVM does not handle stray assumptions very well.

That is, don't just add naively convert all asserts to assumptions and add them for specific optimization purpose. What libc++ ran into was that naive use was bad, and then it worked around with too big of a hammer. See #91801

Just because assumes are bad when used naively doesn't mean they're always bad. This issue is about the interaction of cases when they are valuable, such as informing the compiler about invariants around bounds.

Now, when you write code in C++ or Rust or any other language that encourages abstractions, you will often call small accessors, which have "side effects". They don't actually have side effects, but you'll only discover this after inlining, which you always want in this context.

This issue is about assumptions with "side effects" getting ignored before that inlining. You can work around this by storing things in locals, but the warning is ambiguously phrased and doesn't even make it clear that the assumption is being ignored, just the side effects, but there are no side effects to ignore. At the least, LLVM should rephrase the warning.

[Sirraide]

the warning is ambiguously phrased and doesn't even make it clear that the assumption is being ignored, just the side effects

I see yeah; I suppose the idea behind the warning is that if it worked correctly, you’d get the assumption information, but since the expression is never actually evaluated, then any of its side-effects will be discarded. As it stands, we just discard the entire assumption instead because LLVM can’t actually deal w/ discarding side-effects properly at the moment.

there are no side effects to ignore.

Yeah, we only really make a conservative assumption as to whether something could have side-effects, because this warning here is actually coming from Clang, not LLVM; we don’t actually look at the function body so we don’t know whether or not it has side-effects.

I suppose since we are actually discading the assumption at the moment, it would be more correct to print something along the lines of ‘assumption will be ignored, as it contains side-effects’ and change it back to what is is right now once the backend can actually handle assumptions with side-effects.