aaron.ballman added inline comments.
================
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
----------------
Mordante wrote:
> 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?
>
> 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.
================
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
----------------
Mordante wrote:
> 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?
>
>
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.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:1871
+ ... // The attribute has no effect
+ } while(0); // Clang elides comparison and generates no loop
+
----------------
Mordante wrote:
> 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.
Agreed, good catch!
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
