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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 27 12:44:27 PDT 2020


aaron.ballman added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89899



More information about the cfe-commits mailing list