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

Thu Aug 27 09:43:20 PDT 2020

Mordante planned changes to this revision.
Mordante marked 15 inline comments as done.
Mordante added a comment.

I fixed the issues locally. Before I resubmit them I'd rather discuss in D86559 <https://reviews.llvm.org/D86559> the direction we want to take, so we can avoid an additional review cycle.

================
Comment at: clang/include/clang/Basic/Attr.td:1288
+def Likely : StmtAttr {
+  let Spellings = [CXX11<"", "likely", 201803>];
+  let Documentation = [LikelihoodDocs];
----------------
aaron.ballman wrote:
> Mordante wrote:
> > Mordante wrote:
> > > aaron.ballman wrote:
> > > > Mordante wrote:
> > > > > Mordante wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Mordante wrote:
> > > > > > > > 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?
> > > > > > > > I'll have a look at the C2x changes. Just curious do you know whether there's a proposal to add this to C2x?
> > > > > > > 
> > > > > > > There isn't one currently because there is no implementation experience with the attributes in C. This is a way to get that implementation experience so it's easier to propose the feature to WG14.
> > > > > > > I was also somewhat on the fence, for now I'll change it to specify `1`.
> > > > > > 
> > > > > > There seems to be an issue using the `1` so I used `2` instead. (Didn't want to look closely at the issue.)
> > > > > > 
> > > > > > > I'll have a look at the C2x changes. Just curious do you know whether there's a proposal to add this to C2x?
> > > > > > 
> > > > > > There isn't one currently because there is no implementation experience with the attributes in C. This is a way to get that implementation experience so it's easier to propose the feature to WG14.
> > > > > 
> > > > > Nice to know. It seems the C2x wasn't at straightforward as I hoped, so I didn't implement it yet. I intend to look at it later. I first want the initial part done to see whether this is the right approach.
> > > > What issues are you running into? 1 is the default value when you don't specify a value specifically.
> > > It give the error "clang/include/clang/Basic/Attr.td:265:7: error: Standard attributes must have valid version information."
> > > > > I'll have a look at the C2x changes. Just curious do you know whether there's a proposal to add this to C2x?
> > > > 
> > > > There isn't one currently because there is no implementation experience with the attributes in C. This is a way to get that implementation experience so it's easier to propose the feature to WG14.
> > > 
> > > Nice to know. It seems the C2x wasn't at straightforward as I hoped, so I didn't implement it yet. I intend to look at it later. I first want the initial part done to see whether this is the right approach.
> > 
> > I had another look and I got it working in C.
> If you leave the version number off entirely, it defaults to 1.
Yes and that gives the same error. So the default value doesn't "compile". I assume that's intentional to avoid setting a date of 1 for standard attributes. So we could keep it at 2 or switch back to `201803`. Which value do you prefer?

================
Comment at: clang/include/clang/Basic/AttrDocs.td:1691
+The ``likely`` and ``unlikely`` attributes are used as compiler hints. When
+the next executed statement depends on a condition this attribute can
+annotate all possible statements with either ``likely`` or ``unlikely``.
----------------
aaron.ballman wrote:
> I think this paragraph is misleading because the attribute no longer impacts the statement, it impacts the *entire branch the statement is contained within*.
I'll update the documentation including the other remarks added.

================
Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+  }
+  }];
+}
----------------
aaron.ballman wrote:
> 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.
It doesn't have anything to do with the switch by itself. However while working on the switch I needed a better way to follow the path of executed statements to handle `break` and `return` in a switch. As a side-effect it also handle the last example. So it will be possible to handle this case in a future patch. (However I think we should first discuss the approach to take in D86559.)

================
Comment at: clang/lib/CodeGen/CGStmt.cpp:733
+  if (Else) {
+    LH = LH = getLikelihood(Else);
+    if (LH.first)
----------------
aaron.ballman wrote:
> `LH = LH`?
Has an extra `LH =`, thanks for catching it.

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

https://reviews.llvm.org/D85091