Mordante marked 4 inline comments as done.
Mordante added inline comments.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1726
+ }
+ }];
+}
----------------
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.
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:691
+ Visit(S);
+ if (Result != None)
+ return;
----------------
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.
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:723
+ // The user can use conflicting likelihood attributes within one of the
+ // statements or between the statements. These conflicts are ignored and the
+ // first match is used.
----------------
aaron.ballman wrote:
> I do not think conflicts should result in the first match being used (unless
> we're diagnosing the situation, at which point some of this code should be
> lifted into Sema and set a bit on an AST node that can be read during codegen
> time), so this comment may need to be updated.
Agreed. In my current switch WIP I already started experimenting with moving
the likelihood selection to Sema and store the likelihood in the AST bits.
================
Comment at: clang/lib/CodeGen/CGStmt.cpp:791
+ llvm::MDNode *Weights = nullptr;
+ uint64_t Count = getProfileCount(S.getThen());
+ if (!Count && CGM.getCodeGenOpts().OptimizationLevel) {
----------------
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.
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
