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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+  }
+  }];
+}
----------------
Mordante wrote:
> 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.
> So I think this doesn't make sense and the [[likely]] here will be ignored.

Great, that matches the behavior I was hoping for.

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

Great!

> Not in this patch. I'm working on more improvements to make switch statements working.

I'm not certain what that example had to do with switch statement, but just as an FYI, I'd like this to work initially (assuming it's not overly hard) only because GNU statement expressions show up rather frequently in macros as an alternative form of the `do ... while (0);` pattern, which can have likely/unlikely path sensitivity.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:691
+      Visit(S);
+      if (Result != None)
+        return;
----------------
Mordante wrote:
> 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.
> 
> 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.

That's definitely true and is also a concern of mine. I don't know what the correct answer is here by going on a per-statement basis. None of the solutions are satisfying and all of them leave this feature with surprising behavior.


================
Comment at: clang/lib/CodeGen/CGStmt.cpp:791
+  llvm::MDNode *Weights = nullptr;
+  uint64_t Count = getProfileCount(S.getThen());
+  if (!Count && CGM.getCodeGenOpts().OptimizationLevel) {
----------------
Mordante wrote:
> 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.
> 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.

SGTM


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

https://reviews.llvm.org/D85091