[PATCH] D85091: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in IfStmt

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Aug 17 06:34:26 PDT 2020 

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1288
+def Likely : StmtAttr {
+  let Spellings = [CXX11<"", "likely", 201803>];
+  let Documentation = [LikelihoodDocs];
----------------
Mordante wrote:
> Mordante wrote:
> > aaron.ballman wrote:
> > > Mordante wrote:
> > > > aaron.ballman wrote:
> > > > > Hmm, I'm on the fence about specifying `201803` for these attributes. Given that this is only the start of supporting the attribute, do we want to claim it already matches the standard's behavior? Or do we just want to return `1` to signify that we understand this attribute but we don't yet fully support it in common cases (such as on labels in switch statements, etc)?
> > > > > 
> > > > > As another question, should we consider adding a C2x spelling `[[clang::likely]]` and `[[clang::unlikely]]` to add this functionality to C?
> > > > I was also somewhat on the fence, for now I'll change it to specify `1`.
> > > > 
> > > > I'll have a look at the C2x changes. Just curious do you know whether there's a proposal to add this to C2x?
> > > > I'll have a look at the C2x changes. Just curious do you know whether there's a proposal to add this to C2x?
> > > 
> > > There isn't one currently because there is no implementation experience with the attributes in C. This is a way to get that implementation experience so it's easier to propose the feature to WG14.
> > > I was also somewhat on the fence, for now I'll change it to specify `1`.
> > 
> > There seems to be an issue using the `1` so I used `2` instead. (Didn't want to look closely at the issue.)
> > 
> > > I'll have a look at the C2x changes. Just curious do you know whether there's a proposal to add this to C2x?
> > 
> > There isn't one currently because there is no implementation experience with the attributes in C. This is a way to get that implementation experience so it's easier to propose the feature to WG14.
> 
> Nice to know. It seems the C2x wasn't at straightforward as I hoped, so I didn't implement it yet. I intend to look at it later. I first want the initial part done to see whether this is the right approach.
What issues are you running into? 1 is the default value when you don't specify a value specifically.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1697
+It's not allowed to annotate a statement with both ``likely`` and
+``unlikely``.  It's not recommended to annotate both branches of an ``if``
+statement with an attribute.
----------------
Mordante wrote:
> Mordante wrote:
> > aaron.ballman wrote:
> > > Mordante wrote:
> > > > Quuxplusone wrote:
> > > > > aaron.ballman wrote:
> > > > > > Why? I would expect this to be reasonable code:
> > > > > > ```
> > > > > > if (foo) [[likely]] {
> > > > > >   ...
> > > > > > } else if (bar) [[unlikely]] {
> > > > > >   ...
> > > > > > } else if (baz) [[likely]] {
> > > > > >   ...
> > > > > > } else {
> > > > > >   ...
> > > > > > }
> > > > > > ```
> > > > > > Especially because I would expect this to be reasonable code:
> > > > > > ```
> > > > > > switch (value) {
> > > > > > [[likely]] case 0: ... break;
> > > > > > [[unlikely]] case 1: ... break;
> > > > > > [[likely]] case 2: ... break;
> > > > > > [[unlikely]] default: ... break;
> > > > > > }
> > > > > > ```
> > > > > > As motivating examples, consider a code generator that knows whether a particular branch is likely or not and it writes out the attribute on all branches. Or, something like this:
> > > > > > ```
> > > > > > float rnd_value = get_super_random_number_between_zero_and_one();
> > > > > > if (rnd_value < .1) [[unlikely]] {
> > > > > > } else if (rnd_value > .9) [[unlikely]] {
> > > > > > } else [[likely]] {
> > > > > > }
> > > > > > ```
> > > > > Right, annotating both/multiple branches of a control statement should be perfectly fine. Even `if (x) [[likely]] { } else [[likely]] { }` should be perfectly okay as far as the code generator is concerned (and we should have a test for that); it's silly, but there's no reason to warn against it in the compiler docs.
> > > > > 
> > > > > Aaron, notice that `if (x) [[likely]] { } else if (y) [[likely]] { }` is not actually annotating "both" branches of any single `if`-statement. There you're annotating the true branch of an if (without annotating the else branch), and then annotating the true branch of another if (which doesn't have an else branch).
> > > > > 
> > > > > Mordante, in these docs, please document the "interesting" behavior of the standard attribute on labels — annotating a label is different from annotating the labeled statement itself. In particular,
> > > > > 
> > > > >     [[likely]] case 1: case 2: foo(); break;
> > > > >     case 3: [[likely]] case 4: foo(); break;
> > > > >     case 5: case 6: [[likely]] foo(); break;
> > > > > 
> > > > > indicates that the likely cases are 1, 4, 5, and 6. (1 and 4 because their labels are annotated; 5 and 6 because their control flow passes through an annotated statement. Case 3's control flow passes through an annotated label, but that doesn't matter to the standard attribute.)
> > > > Aaron, the current `CodeGen` code for the `switch` statement allows all branches to have a weight, for the `if` there are only two weights allowed. As Arthur explained the chained `if`s are different statements, so this will work properly.
> > > > 
> > > > Arthur, I agree we should add the documentation about the `switch`, but I'd rather do that when the attributes are implemented for the `switch`. For now I think it would make sense to add some documentation about the fact that the attribute can be placed inside the `CompoundStmt`.
> > > > Aaron, notice that if (x) [[likely]] { } else if (y) [[likely]] { } is not actually annotating "both" branches of any single if-statement. There you're annotating the true branch of an if (without annotating the else branch), and then annotating the true branch of another if (which doesn't have an else branch).
> > > 
> > > I agree, which is why I pointed it out -- I think users are going to be surprised by this behavior because it won't do what they expect it to do.
> > > 
> > > >  As Arthur explained the chained ifs are different statements, so this will work properly.
> > > 
> > > We may have differing definitions of "properly." Users are going to expect to be able to write chains like this:
> > > ```
> > > if (foo) [[likely]] {
> > > } else if (bar) [[likely]] {
> > > } else [[unlikely]] {
> > > }
> > > ```
> > > and have it behave as if they wrote this:
> > > ```
> > > if (foo) [[likely]] {
> > > } else [[likely]] if (bar) [[likely]] {
> > > } else [[unlikely]] {
> > > }
> > > ```
> > > instead of this:
> > > ```
> > > if (foo) [[likely]] {
> > > } else [[unlikely]] if (bar) [[likely]] {
> > > } else [[unlikely]] {
> > > }
> > > ```
> > > in terms of what information gets passed to the optimizer. Given what we currently can support for branch weights, Perhaps the behavior closest to the user's expectation would be as if they wrote this:
> > > ```
> > > if (foo) {
> > > } else if (bar) [[likely]] {
> > > } else [[unlikely]] {
> > > }
> > > ```
> > > (So we say nothing about the first if/else decision branch because the user wants them both to be treated as equally likely.)
> > > > Aaron, notice that if (x) [[likely]] { } else if (y) [[likely]] { } is not actually annotating "both" branches of any single if-statement. There you're annotating the true branch of an if (without annotating the else branch), and then annotating the true branch of another if (which doesn't have an else branch).
> > > 
> > > I agree, which is why I pointed it out -- I think users are going to be surprised by this behavior because it won't do what they expect it to do.
> > > 
> > > >  As Arthur explained the chained ifs are different statements, so this will work properly.
> > > 
> > > We may have differing definitions of "properly." Users are going to expect to be able to write chains like this:
> > > ```
> > > if (foo) [[likely]] {
> > > } else if (bar) [[likely]] {
> > > } else [[unlikely]] {
> > > }
> > > ```
> > > and have it behave as if they wrote this:
> > > ```
> > > if (foo) [[likely]] {
> > > } else [[likely]] if (bar) [[likely]] {
> > > } else [[unlikely]] {
> > > }
> > > ```
> > > instead of this:
> > > ```
> > > if (foo) [[likely]] {
> > > } else [[unlikely]] if (bar) [[likely]] {
> > > } else [[unlikely]] {
> > > }
> > > ```
> > > in terms of what information gets passed to the optimizer. Given what we currently can support for branch weights, Perhaps the behavior closest to the user's expectation would be as if they wrote this:
> > > ```
> > > if (foo) {
> > > } else if (bar) [[likely]] {
> > > } else [[unlikely]] {
> > > }
> > > ```
> > > (So we say nothing about the first if/else decision branch because the user wants them both to be treated as equally likely.)
> > 
> > 
> 
> > We may have differing definitions of "properly." Users are going to expect to be able to write chains like this:
> > ```
> > if (foo) [[likely]] {
> > } else if (bar) [[likely]] {
> > } else [[unlikely]] {
> > }
> > ```
> > and have it behave as if they wrote this:
> > ```
> > if (foo) [[likely]] {
> > } else [[likely]] if (bar) [[likely]] {
> > } else [[unlikely]] {
> > }
> > ```
> > instead of this:
> > ```
> > if (foo) [[likely]] {
> > } else [[unlikely]] if (bar) [[likely]] {
> > } else [[unlikely]] {
> > }
> > ```
> 
> I consider this the "proper" behaviour from the compiler's point of view. I think it's not intended to "taint" the entire path to the attribute. E.g
> ```
> if(foo) { // 1
>    // do stuff
>    if(error) [[unlikely]] { // 2
>        // handle error
>    }
> } else { // 3
>    // do stuff     
> }
> ```
> 
> Here I expect:
> - 1 and 3 to be equally likely
> - 2 to be unlikely
> 
> > in terms of what information gets passed to the optimizer. Given what we currently can support for branch weights, Perhaps the behavior closest to the user's expectation would be as if they wrote this:
> > ```
> > if (foo) {
> > } else if (bar) [[likely]] {
> > } else [[unlikely]] {
> > }
> > ```
> > (So we say nothing about the first if/else decision branch because the user wants them both to be treated as equally likely.)
> 
> I'll give this some more thought and will write some more documentation on what my current approach will do and discuss it from there. It should also take the iteration statements into account and how they handle their likelihood. I'll also have a look at behaviour of GCC and MSVC to see how they've implemented the feature.
> 
> 
> Here I expect:
> 1 and 3 to be equally likely
> 2 to be unlikely

That matches my expectations.

> I'll give this some more thought and will write some more documentation on what my current approach will do and discuss it from there. It should also take the iteration statements into account and how they handle their likelihood. I'll also have a look at behaviour of GCC and MSVC to see how they've implemented the feature.

Thank you! I think my design principle for a binary decision is that repeating the attribute on both branches is valid but a noop because it gives the compiler no additional information, and using opposite attribute on the branches is identical to only one branch being annotated because the information given to the compiler is logically consistent.


================
Comment at: clang/test/SemaCXX/attr-likelihood.cpp:101
+}
+#endif
----------------
Mordante wrote:
> aaron.ballman wrote:
> > Mordante wrote:
> > > Quuxplusone wrote:
> > > > I'd like to see a case like `if (x) [[likely]] i=1;` just to prove that it works on statements that aren't blocks or empty statements. (My understanding is that this should already work with your current patch.)
> > > > 
> > > > I'd like to see a case like `if (x) { [[likely]] i=1; }` to prove that it works on arbitrary statements. This //should// have the same effect as `if (x) [[likely]] { i=1; }`. My understanding is that your current patch doesn't get us there //yet//. If it's unclear how we'd get there by proceeding along your current trajectory, then I would question whether we want to commit to this trajectory at all, yet.
> > > I agree it would be a good idea to add a test like `if (x) [[likely]] i=1;` but I don't feel adding an additional test in this file proves anything. I'll add a test to `clang/test/CodeGenCXX/attr-likelihood-if-branch-weights.cpp` to prove it not only accepts the code but also honours the attribute. This is especially important due to your correct observation that `if (x) { [[likely]] i=1; }` doesn't have any effect. The code is accepted but the CodeGen won't honour the attribute.
> > > 
> > > I think we can use the current approach, but I need to adjust `getLikelihood`. Instead of only testing whether the current `Stmt` is an `AttributedStmt` it needs to iterate over all `Stmt`s and test them for the attribute. Obviously it should avoid looking into `Stmt`s that also use the attribute, e.g:
> > > ```
> > > if(b) {
> > >     switch(c) {
> > >         [[unlikely]] case 0: ... break; // The attribute "belongs" to the switch not to the if
> > >         [[likely]] case 1: ... break; // The attribute "belongs" to the switch not to the if
> > >     }
> > > }
> > > ```
> > > 
> > > This is especially important due to your correct observation that if (x) { [[likely]] i=1; } doesn't have any effect. 
> > 
> > This code should diagnose the attribute as being ignored.
> What I understood from Arthur and rereading the standard this should work, but the initial version of the patch didn't handle this properly.
> What I understood from Arthur and rereading the standard this should work, but the initial version of the patch didn't handle this properly.

My original belief was  that it's acceptable for the attribute to be placed there in terms of syntax, but the recommended practice doesn't apply to this case because there's no alternative paths of execution once you've entered the compound statement. That means:
```
if (x) [[likely]] { i = 1; }
// is not the same as
if (x) { [[likely]] i = 1; }
```
That said, I can squint at the words to get your interpretation and your interpretation matches what's in the original paper (http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0479r2.html#examples).

Ultimately, I think we should strive for consistency between implementations, and I think we should file an issue with WG21 to clarify the wording once we figure out how implementations want to interpret questions like these.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85091/new/

https://reviews.llvm.org/D85091

More information about the cfe-commits mailing list