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

Thu Aug 27 11:22:26 PDT 2020

aaron.ballman added inline comments.

================
Comment at: clang/include/clang/Basic/Attr.td:1288
+def Likely : StmtAttr {
+  let Spellings = [CXX11<"", "likely", 201803>];
+  let Documentation = [LikelihoodDocs];
----------------
Mordante wrote:
> 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?
Ah, yeah, you're right (sorry for the noise). I think `2` is fine for now unless you find that the new patch actually hits enough of the important cases from the standard to justify `201803`. We can figure that out with the updated patch series.

================
Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+  }
+  }];
+}
----------------
Mordante wrote:
> 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.)
Ah, thank you for the explanation.

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

https://reviews.llvm.org/D85091