Page MenuHomePhabricator

[CodeGen] Implement [[likely]] and [[unlikely]] for the iteration statements
ClosedPublic

Authored by Mordante on Oct 21 2020, 10:56 AM.

Diff Detail

Event Timeline

Mordante created this revision.Oct 21 2020, 10:56 AM
Mordante requested review of this revision.Oct 21 2020, 10:56 AM

Thank you for this!

clang/include/clang/Basic/AttrDocs.td
1773

How about: Below are some example usages of the likelihood attributes and their effects:

1854

The loop the likely -> The loop is the likely

Also, mentioning that this applies to range-based for loops may not be a bad thing either.

1861

Oye, I don't know how I feel about this. According to the standard (http://eel.is/c++draft/stmt.pre#1), the unlikely attribute appertains to the compound statement, not the expression within the while loop and that compound statement is required to be entered at least once so this just feels really unintuitive to me. WDYT?

1866

This is a bit of an edge case where I can't tell whether we should or should not diagnose. On the one hand, the user wrote something and we think it's meaningless, which we would usually diagnose as being an ignored attribute so the user can maybe react to it. On the other hand, "has no effect" is perhaps not the same as "ignored", as with i + 0 (it's not really ignored but you would expect it to have no effect either).

1867

elides comparison -> elides the comparison

1871

elides comparison -> elides the comparison

clang/lib/CodeGen/CodeGenFunction.h
1410

attribe -> attribute

Mordante planned changes to this revision.Oct 26 2020, 11:52 AM
Mordante marked 2 inline comments as done.
Mordante added inline comments.
clang/include/clang/Basic/AttrDocs.td
1854

I was doubting whether to mention the range-based for loop separately. I'm not sure how many developers see it as an entirely different for loop. But I'll add an example.

1861

I like the current approach. I agree it feels a bit odd, the initial path of execution will always be the substatement. Whether the substatement will be executed after the condition in the while isn't known upfront so there the likelihood attribute can help. If we don't allow it here I see no other way to express the fact the user expects it unlikely the loop more than one iteration.

I just did a short test with GCC and MSVC and it seems to have no effect on the generated assembly code. So maybe it would be better to remove it for now and discuss it later with the other vendors. Do you agree?

1866

In this case it's ignored since the CodeGen doesn't create a branch instruction. GCC does the same at -O0, MSVC creates the branch at -O0, but removes it at higher optimization levels. So since the other two main compilers also remove the branch I think we could issue a diagnostic in this case we can do that when the CodeGen determines the branch isn't needed. In that case I don't expect false positives. Agreed?

1871

If we keep the do/while likelihood and issue a diagnostic for while(1) we should do the same here.

aaron.ballman added inline comments.Oct 27 2020, 8:15 AM
clang/include/clang/Basic/AttrDocs.td
1861

I just did a short test with GCC and MSVC and it seems to have no effect on the generated assembly code. So maybe it would be better to remove it for now and discuss it later with the other vendors. Do you agree?

I think that's a great approach, thank you!

The reason I am uncomfortable with the current approach is that we went to a lot of effort to make appertainment clear for attributes and *not* let them slide around like GNU-style attributes, and that's effectively what this one is doing. So while the behavior we get is not the worst, it opens the door to more "oh, but I know better than the user where this belongs" sort of shenanigans that we should be resistant to.

1866

I think that's reasonable if it's easy enough for you to do, but I don't insist on a diagnostic if it turns out to be tricky to support for some reason.

1871

Agreed, good catch!

Mordante marked 10 inline comments as done.Oct 27 2020, 12:36 PM

Thanks for the feedback. I'll update the patch after making the requested changes.

clang/include/clang/Basic/AttrDocs.td
1866

Emitting it from the CodeGen is trivial, there it's known whether the branch is emitted. Emitting it from the Sema would be harder, since there it isn't known whether the branch will be omitted. This would mean a -fsyntax-only run won't emit the diagnostic. I'm not sure whether doing the check in the CodeGen affects third-party tools that show diagnostics to the user. For users compiling code it makes no difference.

aaron.ballman added inline comments.Oct 27 2020, 12:44 PM
clang/include/clang/Basic/AttrDocs.td
1866

I don't spend a whole lot of time in CodeGen so I was surprised there were CodeGen-specific diagnostics, but I see now that we do have some and they live with the other frontend diagnostics. I think doing the work from CodeGen is reasonable, but I'm also not certain if third-parties will find it difficult. Given that we already support diagnostics in CodeGen, I'd say it's fine to add another one.

Mordante updated this revision to Diff 301332.Oct 28 2020, 10:52 AM
Mordante marked an inline comment as done.

Addressed the review comments:

  • The likelhood attribute no longer affects the do statement.
  • Added a diagnotic when a likelihood attribute is used on an infinite while loop. Note the diagnostic uses a select since I already use this diagnostic for a WIP patch. That patch issues a diagnostic when the likelihood attribute annotates an if constexpr. Since the attribute has no effect on the do statement there's no warning for do [[likely]] { ... } while(0);. The warning would imply the attribute has an effect on a normal do statement.
  • Various minor fixes.
aaron.ballman accepted this revision.Fri, Oct 30, 9:41 AM

LGTM aside from a request about the note. Thank you for this!

clang/include/clang/Basic/DiagnosticSemaKinds.td
3163

As @rjmccall pointed out in the other review, this isn't really a generic note. I'd mildly recommend renaming the note and removing the select.

This revision is now accepted and ready to land.Fri, Oct 30, 9:41 AM
Mordante marked an inline comment as done.Fri, Oct 30, 11:10 AM

Thanks for the review.

clang/include/clang/Basic/DiagnosticSemaKinds.td
3163

I'll do that and the same for the warn.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.