[PATCH] D89899: [CodeGen] Implement [[likely]] and [[unlikely]] for the iteration statements

[Mark de Wever via Phabricator via cfe-commits]

Mordante marked 10 inline comments as done.
Mordante added a comment.

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



================
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:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89899/new/

https://reviews.llvm.org/D89899