[PATCH] D96462: [LV] Add remarks that explicitly mention error handling in candidate loops

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D96462#2566764 <https://reviews.llvm.org/D96462#2566764>, @fhahn wrote:

> In D96462#2560783 <https://reviews.llvm.org/D96462#2560783>, @jdoerfert wrote:
>
>> In D96462#2556207 <https://reviews.llvm.org/D96462#2556207>, @fhahn wrote:
>>
>>> Thanks for working on improving the remarks! Unfortunately we have to be careful to keep them generic with respect to the frontend. I don't think we should be  using a function name string to detect using `assert` and include other implementation specific parts (like `NDEBUG`) in the message. For example, the user could define their own `__assert_fail` function or the IR could be coming from a Rust or Swift frontend.
>>
>> I'd argue, whatever the frontend or producer is, something called `assert_fail` could be reasonable attributed to "an assertion". If it is implicit or not is a different story but it tells the user more than just: "No unique exit block". Especially if the code doesn't have an obvious side exit (but only an implicit or explicit assertion).
>
> I think it's great you are working on making the remarks more user-friendly!
>
> I'd just like to make sure it works well across a range of frontends. Assuming `__assert_fail` is an assertion in C/C++ is probably fine (unlikely to have many cases where it is not an assertion), but I am still reluctant to encode such frontend-specific logic here. I think a key question we should think about is how other frontends can opt-in to the same remark. I don't have a real suggestion for that at the moment, but if there's anything we can do other than hard-coding assertion function names for x languages here, that would be great.

I don't think Joseph or I are in love with the function name check ;). It's unlikely (IMHO) to cause problems though. I'd love to see a solution that uses a gerneric mechanism, the new annotations (D88645 <https://reviews.llvm.org/D88645>) maybe? So to say a frontend should mark the unreachable/or call before it as "assertion" and here we go.

> Another interesting question is how to best present more context information to the user about remarks. Some of those remarks could benefit from slightly more context/information than can reasonably fit into the remark itself and some way to link to slightly more in-depth documentation would IMO be very valuable. I think such a place would also be slightly better suited to go into frontend/language specific details as well. We could also try to 'guess' the language (based on the file from `!dbg`) and point to language-specific info, if appropriate (e.g. in the `assertion` case)

I agree we need a documentation. For OpenMP we started by giving the remark an id `[OMPXXX]` and the describing it online: https://openmp.llvm.org/docs/remarks/OptimizationRemarks.html
I'd be really happy if we had a centralized remark webpage. I'd like to see example there, and more explanation. Ways to remedy the situation, etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96462