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

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 4 inline comments as done.
Mordante added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+  }
+  }];
+}
----------------
aaron.ballman wrote:
> Something else we should document is what our behavior is when the attribute is not immediately inside of an `if` or `else` statement. e.g.,
> ```
> void func() { // Does not behave as though specified with [[gnu::hot]]
>   [[likely]];
> }
> 
> void func() {
>   if (x) {
>     {
>       [[likely]]; // Does this make the if branch likely?
>       SomeRAIIObj Obj;
>     }
>   } else {
>   }
> }
> 
> void func() {
>   if (x) {
>     int y = ({[[likely]]; /* Does this make the if branch likely? */ 1;});
>   } else {
>   }
> }
> ```
> Something else we should document is what our behavior is when the attribute is not immediately inside of an `if` or `else` statement. e.g.,
> ```
> void func() { // Does not behave as though specified with [[gnu::hot]]
>   [[likely]];
> }

No a few days ago I wondered whether it makes sense, but the [[gnu::hot]] should be at the declaration of the function and not in the body. 
So I think this doesn't make sense and the `[[likely]]` here will be ignored.

> void func() {
>   if (x) {
>     {
>       [[likely]]; // Does this make the if branch likely?
>       SomeRAIIObj Obj;
>     }
>   } else {
>   }
> }

Yes this should work, the attribute will recursively visit compound statements.

> void func() {
>   if (x) {
>     int y = ({[[likely]]; /* Does this make the if branch likely? */ 1;});
>   } else {
>   }
> }
> ```

Not in this patch. I'm working on more improvements to make switch statements working. I tested it with my current WIP and there it does work.
So in the future this will work.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:691
+      Visit(S);
+      if (Result != None)
+        return;
----------------
aaron.ballman wrote:
> Is early return the correct logic? Won't that give surprising results in the case the programmer has both attributes in the same compound statement? I think we need to look over all the statements in the current block, increment a counter when we hit `[[likely]]`, decrement the counter when we hit `[[unlikely]]` and return whether the counter is 0, negative (unlikely), or positive (likely).
Yes here it accepts the first likelihood it finds and accepts that as the wanted likelihood. I also start to doubt whether that's wanted. In my current switch WIP I  issue an diagnostic if there's a conflict between the likelihood diagnostics and ignore them. I feel that's a better approach. This diagnostic is added to the `-Wignored-attributes` group, which is shown by default.

I don't like the idea of using a counter. If the attribute is "hidden" in a validation macro, adding it to a branch might suddenly change the likelihood of that branch.



================
Comment at: clang/lib/CodeGen/CGStmt.cpp:723
+  // The user can use conflicting likelihood attributes within one of the
+  // statements or between the statements. These conflicts are ignored and the
+  // first match is used.
----------------
aaron.ballman wrote:
> I do not think conflicts should result in the first match being used (unless we're diagnosing the situation, at which point some of this code should be lifted into Sema and set a bit on an AST node that can be read during codegen time), so this comment may need to be updated.
Agreed. In my current switch WIP I already started experimenting with moving the likelihood selection to Sema and store the likelihood in the AST bits.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:791
+  llvm::MDNode *Weights = nullptr;
+  uint64_t Count = getProfileCount(S.getThen());
+  if (!Count && CGM.getCodeGenOpts().OptimizationLevel) {
----------------
aaron.ballman wrote:
> Perhaps not for this patch, but I wonder if we'd be doing users a good deed by alerting them when PGO weights do not match the attribute specified. e.g., if an attribute says "this branch is likely" and PGO shows it's unlikely, that seems like something the programmer may wish to know. WDYT?
I already investigated before and there's a diagnostic `warn_profile_data_misexpect` when using `__builtin_expect` so I expect I can reuse existing code. So I want to have a look at it, but I first want to get the more important parts of the likelihood attributes working properly.


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

https://reviews.llvm.org/D85091