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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
