[PATCH] D86559: [Sema, CodeGen] Allow [[likely]] and [[unlikely]] on labels

Sat Oct 31 10:40:28 PDT 2020

Mordante added inline comments.

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6454

+static bool validateLikelihoodAttr(Sema &S, Decl *D, const ParsedAttr &A) {
+  if (!isa<LabelDecl>(D)) {
----------------
Mordante wrote:
> aaron.ballman wrote:
> > Mordante wrote:
> > > aaron.ballman wrote:
> > > > This is entering into somewhat novel territory for attributes, so some of this feedback is me thinking out loud.
> > > > 
> > > > Attr.td lists these two attributes as being a `StmtAttr` and not any kind of declaration attribute. We have `DeclOrTypeAttr` for attributes that can be applied to declarations or types, but we do not have something similar for statement attributes yet. We do have some custom semantic handling logic in SemaDeclAttr.cpp for statement attributes, but you avoid hitting that code path by adding a `case` for the two likelihood attributes. These attributes only apply to label declarations currently, and labels cannot be redeclared, so there aren't yet questions about whether this is inheritable or not. So we *might* be okay with this, but I'm not 100% certain. For instance, I don't recall if the pretty printer or AST dumpers will need to distinguish between whether this attribute is written on the statement or the declaration (which is itself a bit of an interesting question: should the attribute attach only to the statement rather than trying to attach to the underlying decl? http://eel.is/c++draft/stmt.stmt#stmt.label-1.sentence-2 is ambiguous, but I don't think of `case` or `default` labels as being declarations so I tend to not think of identifier labels as such either.). There's a part of me that wonders if we have a different issue where the attribute is trying to attach to the declaration rather than the statement and that this should be handled purely as a statement attribute.
> > > > 
> > > > I'm curious what your thoughts are, but I'd also like to see some additional tests for the random other bits that interact with attributes like AST dumping and pretty printing to be sure the behavior is reasonable.
> > > The labels in a switch are indeed different and the code in trunk already should allow the attribute there. (I'm still busy with the CodeGen patch.)
> > > I agree that Standard isn't clear whether the attribute is attached to the statement or the declaration.
> > > 
> > > The `LabelDecl` expects a pointer to a `LabelStmt` and not to an `AttributedStmt`. Since declarations can already have attributes I used that feature. I just checked and the `LabelDecl` isn't shown in the AST and so the attributes also aren't shown. I can adjust that.
> > > 
> > > Another option would be to change the `LabelDecl` and have two overloads of `setStmt`
> > > `void setStmt(LabelStmt *T) { TheStmt = T; }`
> > > `void setStmt(AttributedStmt *T) { TheStmt = T; }`
> > > Then `TheStmt` needs to be a `Stmt` and an extra getter would be required to get the generic statement.
> > > 
> > > I think both solutions aren't trivial changes. Currently the attribute has no effect on labels so it not being visible isn't a real issue. However I feel that's not a proper solution. I expect attributes will be used more in C and C++ in the future. For example, I can imagine a `[[cold]]` attribute becoming available for labels.
> > > 
> > > So I'm leaning towards biting the bullet and change the implementation of `LabelDecl` to allow an `AttributedStmt` instead of a `LabelStmt`.
> > > WDYT?
> > > Currently the attribute has no effect on labels so it not being visible isn't a real issue. 
> > 
> > That's not entirely true though -- we have pretty printing capabilities that will lose the attribute when written on a label, so round-tripping through the pretty printer will fail. But we have quite a few issues with pretty printing attributes as it stands, so I'm not super concerned either.
> > 
> > > So I'm leaning towards biting the bullet and change the implementation of LabelDecl to allow an AttributedStmt instead of a LabelStmt.
> > > WDYT?
> > 
> > I'm curious if @rsmith feels the same way, but I think something along those lines makes sense (if not an overload, perhaps a templated function with SFINAE). We'd have to make sure that the attributed statement eventually resolves to a `LabelStmt` once we strip the attributes away, but this would keep the attribute at the statement level rather than making it a declaration one, which I think is more along the lines of what's intended for the likelihood attributes (and probably for hot/cold if we add that support later).
> > > Currently the attribute has no effect on labels so it not being visible isn't a real issue. 
> > 
> > That's not entirely true though -- we have pretty printing capabilities that will lose the attribute when written on a label, so round-tripping through the pretty printer will fail. But we have quite a few issues with pretty printing attributes as it stands, so I'm not super concerned either.
> 
> I'll keep that in mind when I start working on that.
> 
> > 
> > > So I'm leaning towards biting the bullet and change the implementation of LabelDecl to allow an AttributedStmt instead of a LabelStmt.
> > > WDYT?
> > 
> > I'm curious if @rsmith feels the same way, but I think something along those lines makes sense (if not an overload, perhaps a templated function with SFINAE). We'd have to make sure that the attributed statement eventually resolves to a `LabelStmt` once we strip the attributes away, but this would keep the attribute at the statement level rather than making it a declaration one, which I think is more along the lines of what's intended for the likelihood attributes (and probably for hot/cold if we add that support later).
> 
> Yes if we go for an overload I need to make sure that the attributed statement has a `LabelStmt` as its substatement. I haven't looked into how to enforce that.
> 
> @rsmith Any opinion on whether the likelihood attribute should be "attached" to the label declaration or the label statement?
> So I'm leaning towards biting the bullet and change the implementation of `LabelDecl` to allow an `AttributedStmt` instead of a `LabelStmt`.

Actually it seems this isn't required, it works when `ActOnLabelStmt` returns an `AttributedStmt`. This means the `LabelDecl` class doesn't need to change.

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

https://reviews.llvm.org/D86559