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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


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


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2402
+def note_attribute_compatibility_here : Note<
+  "attribute '%0' declarered here">;
+def warn_attribute_likelihood_if_duplicated : Warning<
----------------
Typo: /declarered/declared/


================
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;
----------------
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...)


================
Comment at: clang/lib/Sema/SemaStmtAttr.cpp:349
+            << Unlikely->getSpelling() << Unlikely->getRange();
+
+        return;
----------------
Nit: Remove this blank line, for parallelism with lines 358-359.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85091