Mordante planned changes to this revision.
Mordante marked 2 inline comments as done.
Mordante added inline comments.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1854
+ for(int i = 0; i != size; ++i) [[likely]] {
+ ... // The loop the likely path of execution
+ }
----------------
aaron.ballman wrote:
> 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.
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.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1861
+
+ do [[unlikely]] { // The loop will always iterate once
+ ... // The likelihood attribute affects the likelihood of the
----------------
aaron.ballman wrote:
> 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?
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?
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1866
+ while(true) [[unlikely]] {
+ ... // The attribute has no effect
+ } // Clang elides comparison and generates an infinite loop
----------------
aaron.ballman wrote:
> 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).
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?
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1871
+ ... // The attribute has no effect
+ } while(0); // Clang elides comparison and generates no loop
+
----------------
aaron.ballman wrote:
> elides comparison -> elides the comparison
If we keep the do/while likelihood and issue a diagnostic for `while(1)` we
should do the same here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89899/new/
https://reviews.llvm.org/D89899
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
