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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for starting the implementation work on these attributes!



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


================
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.
----------------
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]] {
}
```


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1702
+
+At the moment the attribute is only implemented for an ``if`` statement.
+
----------------
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).


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1708
+
+  // compile with -Wimplicit-fallthrough
+  if (b) [[likely]] {
----------------
Why does `-Wimplicit-fallthrough` matter?


================
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,
----------------
Is this because you expect people will want to control diagnostics on `if` statements separate from diagnostics in other syntactic locations?


================
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'">;
----------------
This isn't needed, we already have `err_attributes_are_not_compatible`.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2401
+  "incompatible attributes '%0' and '%1'">;
+def note_attribute_compatibility_here : Note<
+  "attribute '%0' declarered here">;
----------------
We've already got `note_conflicting_attribute` for this as well.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:658
+  auto R = std::make_pair(false, false);
+  auto *AS = dyn_cast<AttributedStmt>(Stmt);
+  if (!AS)
----------------
`const auto *`


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85091