[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#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).



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1078
+            "loop control flow is not understood by vectorizer. "
+            "Loop contains control flow that does not return",
+            "CFGNotUnderstood", ORE, TheLoop, U);
----------------
jhuber6 wrote:
> fhahn wrote:
> > In this case, the loop *technically* does not contain the `unreachable`, right? One of the exit blocks does. I am not sure how to best phrase it, but it might be worth adjusting the message below `reportVectorizationFailure("The loop must have a unique exit block",` to include the information that some of the exit blocks end in `unreachable`
> > 
> > Unfortunately I don't think the user can do much with this information.
> Yes, the loop shouldn't contain an `unreachable` instruction, but the error handling code will be inside the loop and will create a new exit block to handle the error. This is just a specialization of failing because of multiple exit blocks. The main benefit of checking it separately here is that the diagnostics will point to the expression that caused it. Some of these situations have simple solutions, such as recompiling without assertions or doing manual loop unswitching.
> 
> Also. should the remarks be prefaced with "loop control flow is not understood by vectorizer." In this case it will print another remark saying the same anyway.
> Unfortunately I don't think the user can do much with this information.

I don't believe that. I think the key point is that such unreachable exit blocks are often "hidden" from the user. Calling exit, abort, assert, or having implicit range checks, doesn't look like a break or return on the source level. Once we tell people what the problem with the control flow is in terms that actually map to the source, e.g., error handling, assertions, etc. they can act on it. IMHO, "no unique exit block" is not tangible if you have:
```
for (int i = 0; i < N; ++i) {
  A[i] = ...;
}
```
with `T& decltype(A)::operator[](int i) { assert(i < A.size()); return data[i]; }`


> In this case, the loop *technically* does not contain the unreachable, right?

Right. We could say something along the lines of:

"Loop exit block contains control flow that does not return, e.g., error handling"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96462