[RFC] __builtin_assume, [[assume]] optimization behavior

Clang Frontend
21

I think this is more of a standard issue, or WG21 issue.

Yeah. Completely agreed. If the register can be deprecated and removed in the standard level, why can’t we perform the same thing for [[assume]]?

My impression for standardized feature like [[assume]] and [[likely]]/[[unlikely]] is:

  • There are already the extensions in the 3 compilers.
  • It looks like it works pretty well.
  • Let’s see if we can lift them into the standardization level.

I feel the gap between the second step and the third step explains the argument from @nikic. It is true that the @llvm.assume works well in practices. But it doesn’t fit to the quality of standardization to me.

The problem may be that we take an oversight in the third step. For the WG21 side, we should require the WG21 to review the issue. For the vendor’s side, we should ignore [[assume()]] until the LLVM part provides a better version. For __builtin_assume , we should leave it as is since it is on the second step.

Yes, this is wanted. But we can’t provide everything our users want. We have limited capabilities. As you said, it is more of a design issue. For the design issue, we’d better to discuss this in WG21.

For users who still want it (called as expert users in the thread), they can still use __builtin_assume.

Yes, agreed. It is indeed confusing and not easy for end users to understand easily. But I think this may be the price we need to pay due to we don’t notice such issue in the earlier days.

22

We should (have) made this a GSoC project. Just trying out when/where assume in IR is bad, having data would help us to focus. And maybe if we can use the encoding suggestions from 2019 to represent all the current use cases w/o the negative impact.

4 Likes
23

Hah! This was just standardized a few months ago, and in fact, most of the concerns brought up in this thread were brought up and the response from the committee was an overwhelming acceptance of those limitations.

I’m not sure what else we can discuss at WG21. Implementers brought up ALL of these concerns in Evolution in WG21. As an EWG chair, I don’t see any ‘new’ information that gives us justification for re-opening the discussion.

This is something they want despite the design issues. We have the implementation available (as in, we have an implementation under review missing only a few parts). Again, the ‘design issue’ is a closed subject in WG21.

Seemingly, the point that was made is that this is insufficient in a way that only [[assume]] can fix :slight_smile:

1 Like
24

I voted “option 3” in the poll, and then read Aaron’s argument for option #1, and I think it’s pretty compelling. I am particularly compelled by Microsoft’s comments, and the point that assumptions are a new way to induce undefined behavior. To me, those are strong reasons to just say no and not implement this feature.

As I understand it, GCC hasn’t implemented __builtin_assume, I haven’t heard they are planning to implement [[assume]], and Microsoft has said explicitly that they will not, which begs the question, why should Clang? We’d be no better off than we are with __builtin_assume today, unless we get new information that GCC plans to implement this feature. If there was ever a feature from WG21 that implemeters could collectively say “no” to, this seems like it could be the one!

I don’t find the arguments around performance expectations that compelling, despite my involvement in reporting the problems we had in libc++. My view is that performance changes with every compiler update as the optimizer changes. Performance does not get unilaterally better for every workload. Particular usages have performance regressions, while overall performance should improve. To the extent that users care about performance, they will have some benchmarking and performance monitoring infrastructure, and this is enough of a feedback loop to discover assumptions that act as optimization barriers.

I don’t like the idea of a blanket warning on all assumptions. That just seems like it’s going to create friction for intentional usage of the feature. I wonder if it would instead be possible to create optimizer remarks to let users know that a particular assumption was never used, was discarded, or was considered low value. This would probably be unhelpful for cases where the assumption is meant to convey interprocedural information (I think this was the intention for libc++), but I think it’s much better to model IPO hints with function, argument, and return attributes. Overall, assumptions are finnicky feature for expert performance engineers, and if we want to support this use case, the compiler needs to be more communicative than to silently generate slightly different code.

My last idea is that perhaps the compiler should have a flag that disables all assumption tracking. This makes it really easy check if assumptions are helping at all for any particular benchmark, and it also makes it easy to disable any undefined behavior footguns if that’s something you are worried about. For example, Chrome, a networked client application, clearly needs to have a different UB security posture than, say, a 3D rendering application, and would probably use this feature.

1 Like
25

A flag to emit asserts for [[assume]] and __builtin_assume, instead of optimization hints, to verify that the assumes are correct at runtime might help alleviate some footgun concerns. Could even unconditionally do this for -O0.

2 Likes
26

I don’t know about __builtin_assume(), but GCC does seem to support [[assume]] and use it as an optimisation hint: Compiler Explorer

27

I don’t think it has been mentioned yet, but another approach would be to allow a command line option to select a semantic for the [[assume]] attribute.

The [[assume]] attribute has conceptual overlap with C++ contracts as detailed in P2900 (Contracts for C++); see section 3.5.2, “Contract Semantics: ignore, enforce, observe”. Though that paper only specifies three semantics at present, it acknowledges a fourth assume semantic that is expected to be proposed in the future.

The C++ contracts semantics can be mapped to the [[assume]] attribute as follows. In each of these cases, __has_cpp_attribute(assume) would evaluate to 202207L (or later).

  • ignore: The [[assume]] attribute is ignored and has no impact on the generated LLVM IR.
  • enforce: The [[assume]] attribute is treated as an assertion (perhaps as a contract_assert in the future; see section 3.2.2, “Assertion statement”, of P2900), and execution is halted (if the invoked contract violation handler returns).
  • observe: The [[assume]] attribute is treated as an assertion (perhaps as a contract_assert in the future; see section 3.2.2, “Assertion statement”, of P2900), and execution continues (if the invoked contract violation handler returns).
  • assume: The [[assume]] attribute is treated as an assumption, and LLVM IR generation is altered as-if by __builtin_assume.

A command line option to control the behavior might look like:
-fassume-semantic=[ignore|enforce|observe|assume]. We could start with the default being ignore and then, based on positive reports of projects using assume, change the default in a future release.

5 Likes
28

Thank you for giving this suggestion Tom, it’s not an approach that we had considered before. I’ve been thinking about this for a few days and I actually really like this approach as it puts the decision into the hands of users and gives them an appropriate amount of knobs to tweak that behavior. It also side-steps the desire for an on-by-default diagnostic to warn users about the dangers of the feature.

I can see a direct mapping of ignore as a noop and assume to __builtin_assume calls. enforce or observe would be a bit different because of [dcl.attr.assume]. e.g., I don’t think we can do:

[[assume(foo() && bar + 12 == 100)]]; // Original code

// Lowers to:
if (!(foo() && bar + 12 == 100)) {
  fputs("assumption didn't hold at runtime", stderr);
  abort(); // Only if in 'enforce' mode
}

unless we’re willing to say that -fassume-semantic=enforce|observe is a language dialect rather than a standards conforming mode. Because it’s an explicit opt-in, I think that’s fine to do so, but in practical terms, it means that our default will only ever be ignore or assume for conformance reasons.

@Sirraide as the patch author and @erichkeane as attributes code owner: would you be in support of Tom’s suggestion? If so, do you prefer it over the existing Option #3 (which is what currently has the most support of the original options we considered)?

2 Likes
29

Something to keep in mind here is that the most important users of [[assume]] are likely going to be libraries, not end users. The end user, who determines the flags the project is compiled with, will not know and shouldn’t know about the specifics of [[assume]] use inside libraries.

It stands to reason that [[assume]] conditions will be checked under -fsanitize=undefined just like everything else, but beyond that I’m not convinced that having an -fassume-semantic option would be particularly useful.

3 Likes
30

Thanks for the suggestion Tom.
Providing an on/off switch for assume maybe make sense to me. (ie it gives people an escape hatch if LLVM pessimizes assume to an extent it causes problems,
we could spell it -fno-experimental-assume)

I am opposed to the enforce/observe semantics you propose.
[[assume]] was purposefully decoupled from contracts because

  • We did not want assertions to guide optimizations
  • We did not want people to use [[assume]] as a way to spell assert.

Deviating from that will create a lot of confusion when/if contracts land, and I hope clang does not add to the confusion.
In particular, I am concerned about people using [[assume]] as a way to assert.

This is a generalization of the assert(false) vs unreacheable question.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2064r0.pdf has more background on that distinction.

I do think that a way to detect bogus assumptions would be nice, but I think the way to do that would be to add support for that in -fsanitize=undefined,
and not by trying to invent contracts semantics that might not be reconcilable with other implementations / future C++ standards

1 Like
31

The end user needs to be able to control those specifically because the library author has no way of knowing whether the application’s use of the library facilities is optimized or pessimized by the assumptions. So I would argue that using assumptions in (the header interface of) libraries is made more useful by having this kind of control.

32

Thanks for the feedback! I’d also be quite happy with supporting only the ability to ignore or lower assumptions to LLVM IR and not having enforce or observe semantics for them. However, I think those other modes do have useful applications for testing and debugging purposes. But we could always extend the list of options in the future based on user needs too, so I don’t think they need to be there up front.

One thing I want to keep in mind though is that WG21 has been working on contracts for many years and are still debating what makes an MVP. I don’t think we should hold back functionality on the assumption (pun absolutely intended) that WG21 will ship contracts or ship them in their current form. That said, we definitely don’t want to make things harder for WG21 to ship contracts, so it’s good to keep that background information in mind (thank you for sharing it).

33

I think I’m against Tom’s suggestion for 3 reasons:
1- Its pretty complicated, and is going to result in a significant number of code-gen-esque dialects that are going to be a pain to run through, and most are going to be incredibly rarely used. Providing ‘knobs’ here just offloads the responsibility for the poor decision on the end-user.

2- @nikic’s concerns resonate with me.

3- This feature was sold to EWG as ‘we need this even without contracts, because it isn’t contracts’, so making it THIS much like contracts makes me prefer just not implementing it at all. So my preference would be: Option #3, Option #1, this.

1 Like
34

Ok, speaking offline with @AaronBallman and @cor3ntin, hopefully this is acceptable to @Sirraide, as the implementer

I think (and I believe the other 2 agree) that we’re all in agreement that a flag is a positive improvement here, but we also think that there are sufficient downsides/disagreement as to the full suite of flags suggested by Tom.

We believe that a -fassume-semantics=ignore|assume (perhaps spelled in a way that doesn’t imply it modifies the builtin’s behavior), which chooses between Option#1 and option #3 (where #1 is ‘implement the whole attribute, but skip codegen’ but leave the feature-test-macro as 0, and option #3 is to update the macro and do the codegen) is an effective and usable way forward.

As a SIDE note, teaching UBSan about this attribute is, in our opinions, a fantastic idea that gets the benefits of the other flags, and does so in a way that already works with our current infrastructure/tooling.

However, this has interesting implications between UBSan being run/not run in the cases of an assume with side effects (which I believe can be diagnosed), so I don’t believe the UBSan implementation is a blocker for this implementation, and can be done separately (and not required out of @Sirraide ).

3 Likes
35

I’m in agreement, the full suite of flags is interesting but the critical functionality I would like to see is letting the user control whether assumptions cause undefined behavior for the optimizer to reason with.

Agreed – I would love to see UBSan support diagnosing incorrect assumptions, but I think it’s orthogonal to the initial support for the attribute.

36

I agree with others that the ignore and assume semantics are the most interesting ones to implement for the immediate future. The observe and enforce cases can be reconsidered after support for contracts is added.

I agree a ubsan option to validate assumptions is interesting.

For the ignore behavior, I think the feature test macro should still have a non-0 value. Otherwise, use of the ignore option won’t have the intended effect for code like the following:

#if __has_cpp_attribute(assume)
  #define ASSUME(X) [[assume(X)]]
#else
  #define ASSUME(X) __builtin_assume(X)
#endif

If a mode is desired to have __has_cpp_attribute(assume) be 0, then I think that should be specified via a different option. Perhaps -fassume-semantic=disabled.

1 Like
37

For the ignore behavior, I think the feature test macro should still have a non-0 value. Otherwise, use of the ignore option won’t have the intended effect for code like the following:

I disagree and don’t have sympathy for that coding pattern, if someone wishes to do that, than they can deal with whether they want to set it as a flag or not.

But NOT emitting codegen and setting the FTM is a non-starter for me, that doesn’t conform to the (albeit in a note) standard. I’m accepting of a version where we can switch between the two standards blessed implementations, or either one of the standard blessed implementations, but not a situation that enables us to do neither.

1 Like
38

For reference, I believe the relevant note is Note 1 in [dcl.attr.assume].

Personally, I think WG21 got this wrong and that the feature test macro should only be used to test if the implementation recognizes the attribute. But my vote landed on the minority side of the poll.

1 Like
39

You and I voted on the same side of that poll, but this is a ‘disagree and commit’ in my mind.

3 Likes
40

I’m not entirely opposed to implementing it, though I still prefer option #3 (i.e. simply lowering assumptions to LLVM IR unconditionally and letting the user ‘deal with the consequences’—for better or for worse). In addition to the concerns that were already pointed out, I’d also like to add that adding a Clang-specific flag to tune assumption behaviour might also just end up exacerbating this already fairly confusing situation, because now there’s another thing you have to keep in mind wrt assumptions.

I am likewise opposed to an enforce/observe semantics, as that really just seems like it would be conflating assumptions with contracts. At the same time, I’d argue that it would still make sense to check it 1. at compile time and 2. (as @cor3ntin suggested) if ubsan is enabled, as violating an assumption is basically just another form of UB—which is why it would make sense to diagnose it in either case.

As I said, I’m not opposed to this option; it should be fairly straight-forward to implement. On that note, would assume or ignore be the default?

I do wonder if returning 0 for the feature-test macro is really the best idea if we decide to do it this way. Basically, I’m picturing a situation where someone wants to turn off assumptions because they know about this problem, sets the flag to ignore, which causes a __has_cpp_attribute check in a header to return 0, which then activates a fallback to __builtin_assume or something similar, which is probably not what they had intended. At the same time, this might well be too contrived… (though, perhaps not; I just saw that @tahonermann already brought up similar concerns)

Moreover, returning 0 in that case might be required (or ‘expected’, to quote [dcl.attr.assume]) by the standard. I guess you could argue on a technicality that, if not emitting assumptions avoids a pessimisation, then, in a sense, that would be an optimisation, and we’d be complying with the standard after all, but as @AaronBallman already pointed out when we were discussing the RFC, this would be a somewhat malicious reading of the standard, and it candidly wouldn’t quite sit right with me to do it that way…

Also, are you proposing that this flag should affect __builtin_assume as well? If so, that could remedy the situation I described above, and it would also just make sense imo, seeing as I can’t really think of a reason why someone would want to ‘turn off’ [[assume]] but not also __builtin_assume.

Yeah, this sounds like it should be a separate pr—and I’m also candidly not familiar with the inner workings of the sanitisers at all…

So yeah, in sum, while I personally am still in favour of option #3, adding a flag sounds like a reasonable alternative, and I’m not opposed to implementing it if that’s the consensus we’ve reached here (though I would personally argue that, if we’re adding a flag, it should affect __builtin_assume as well, for consistency).

2 Likes