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

[Mark de Wever via Phabricator via cfe-commits]

Mordante planned changes to this revision.
Mordante marked 14 inline comments as done.
Mordante added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1288
+def Likely : StmtAttr {
+  let Spellings = [CXX11<"", "likely", 201803>];
+  let Documentation = [LikelihoodDocs];
----------------
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?


================
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.
----------------
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`.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1702
+
+At the moment the attribute is only implemented for an ``if`` statement.
+
----------------
aaron.ballman wrote:
> We should document whether we silently accept the attribute in other locations and do nothing with it (I'm not keen on this approach because it surprises users) or that we loudly diagnose the attribute in situations that we don't support it in (I think this is a more user-friendly approach, but may require a separate warning flag).
I'll update the documentation. I intend to continue to work on implementing these attributes so I feel adding a temporary diagnostic is not worth the effort.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1708
+
+  // compile with -Wimplicit-fallthrough
+  if (b) [[likely]] {
----------------
aaron.ballman wrote:
> Why does `-Wimplicit-fallthrough` matter?
It doesn't, thanks for spotting the copy-paste error.


================
Comment at: clang/include/clang/Basic/DiagnosticGroups.td:659
 def IgnoredAttributes : DiagGroup<"ignored-attributes">;
+def LikelihoodAttributeIf : DiagGroup<"likelihood-attribute-if">;
 def Attributes : DiagGroup<"attributes", [UnknownAttributes,
----------------
aaron.ballman wrote:
> Is this because you expect people will want to control diagnostics on `if` statements separate from diagnostics in other syntactic locations?
This is about warning about `[[likely]]` in both branches. Due to your an Arthur's remarks I've decided to remove them. Especially since Arthur correctly pointed out the attribute doesn't need to be directly after the `if(x)`.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2399
 def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
+def err_attribute_compatibility : Error<
+  "incompatible attributes '%0' and '%1'">;
----------------
aaron.ballman wrote:
> This isn't needed, we already have `err_attributes_are_not_compatible`.
Thanks I didn't find them before, removed.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2401
+  "incompatible attributes '%0' and '%1'">;
+def note_attribute_compatibility_here : Note<
+  "attribute '%0' declarered here">;
----------------
aaron.ballman wrote:
> We've already got `note_conflicting_attribute` for this as well.
Thanks I didn't find them before, removed.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:679-680
+
+  // The code doesn't protect against the same attribute on both branches.
+  // The frontend already issued a diagnostic.
+  std::pair<bool, bool> LH = getLikelihood(Then);
----------------
aaron.ballman wrote:
> I don't think the frontend should issue a diagnostic for that situation. I think codegen should handle chains appropriately instead as those seem plausible in reasonable code.
This is the diagnostic about `if(b) [[likely]] {...} else [[likely]] {...}`. I'll remove the diagnostic, however the CodeGen will still prioritize the `[[likely]]` attribute in the `ThenStmt`. With Arthur's information the user can even do more unwise things:
```
if(b) [[likely]] {
    [[unlikely]] ...
} else [[likely]] {
    [[unlikely]]...
}
```
If the user provided conflicting attributes the CodeGen will not fail, but might not do what the user wanted.
This doesn't affect the chained `if`:
```
if(b) [[likely]] {
    ...
} else if(c) [[likely]] {
    ...
}
```

For the `switch` it will be allowed to annotate multiple `case`s with the same attribute and that should work as intended. (In a `case` it won't 'protect' against conflicting attributes in one `case`).


================
Comment at: clang/lib/Sema/SemaStmt.cpp:585
+  // Warn when both the true and false branch of an if statement have the same
+  // likelihood attribute. It's not prohibited, but makes no sense.
+  const LikelyAttr *Likely = nullptr;
----------------
Quuxplusone wrote:
> I agree with Aaron, this doesn't deserve a warning. Or at least there should be some thought put into the "threat model." (Are we protecting against someone typoing `[[likely]]`/`[[likely]]` when they meant `[[likely]]`/`[[unlikely]]`? But they weren't supposed to annotate both branches in the first place...)
Yes it protects against `[[likely]]/[[likely]]`. I've decided to remove the warning.


================
Comment at: clang/test/SemaCXX/attr-likelihood.cpp:101
+}
+#endif
----------------
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
    }
}
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85091