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

[jhuber6]

Summary

Some loops contain assertions or other methods of error handling that the user might not be aware of. For example, if the access operator for a custom matrix class had bounds checking. The vectorizer's remarks should be more specific if a loop was not deemed legal for this reason. This patch adds a remark that specifically mentions how to prevent assertions from preventing vectorization, and a more generic response for any control flow that will not return.

[fhahn]

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.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

1085

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.

[jhuber6]

In 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.

The motivation to handle __assert_fail specifically as a secondary case is mainly because it has a simple solution we can put in the message. It might be sufficient to only check for unreachable instructions because the diagnostics message should point to the offending assertion and leave it to the user to know how to turn them off.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

1085

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.

[jdoerfert]

In 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).

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

1085

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"

[fhahn]

In D96462#2560783, @jdoerfert wrote:

In 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.

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)

[fhahn]

(I'm adding Francis & Adam, because they've been working on remarks a lot and perhaps have additional insight/suggestions)

[jdoerfert]

In D96462#2566764, @fhahn wrote:

In D96462#2560783, @jdoerfert wrote:

In 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) 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.

[jhuber6]

Removing check for assert to keep the pass agnostic to the front-end and cleaning up the test file.

[jhuber6]

Ping.

[jhuber6]

Should I just abandon this? Try to clear out my open revisions.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

if (blockNeedsPredication(BB)) {
       return false;
     }
   }
 
   // We can if-convert this loop.
   return true;
 }
 
+// Check for unreachable instructions in the loop to emit a remark on.
+static void checkErrorHandling(SmallVector<BasicBlock *> &ExitBBs,
+                               Loop *TheLoop, OptimizationRemarkEmitter *ORE) {
+  for (BasicBlock *BB : ExitBBs) {
+    for (Instruction &Inst : *BB) {
+      if (auto *U = dyn_cast<UnreachableInst>(&Inst)) {
+        reportVectorizationFailure(
+            "The loop must have no error handling",
+            "loop exit block contains control flow that does not return",
+            "CFGNotUnderstood", ORE, TheLoop, U);
+        return;
+      }
+    }
+  }
+}
+
 // Helper function to canVectorizeLoopNestCFG.
 bool LoopVectorizationLegality::canVectorizeLoopCFG(Loop *Lp,
                                                     bool UseVPlanNativePath) {
   assert((UseVPlanNativePath || Lp->isInnermost()) &&
          "VPlan-native path is not enabled.");
 
   // TODO: ORE should be improved to show more accurate information when an
   // outer loop can't be vectorized because a nested loop is not understood or
   // legal. Something like: "outer_loop_location: loop not vectorized:
   // (inner_loop_location) loop control flow is not understood by vectorizer".
 
reportVectorizationFailure("The loop must have a single backedge",
         "loop control flow is not understood by vectorizer",
         "CFGNotUnderstood", ORE, TheLoop);
     if (DoExtraAnalysis)
       Result = false;
     else
       return false;
   }
 
+  // Check that this loop does not contain error handling blocks
+  if (!Lp->getUniqueExitBlock() && DoExtraAnalysis) {
+    SmallVector<BasicBlock *> ExitBBs;
+    Lp->getExitBlocks(ExitBBs);
+    checkErrorHandling(ExitBBs, TheLoop, ORE);
+  }
+
   // We currently must have a single "exit block" after the loop. Note that
   // multiple "exiting blocks" inside the loop are allowed, provided they all
   // reach the single exit block.
   // TODO: This restriction can be relaxed in the near future, it's here solely
   // to allow separation of changes for review. We need to generalize the phi
   // update logic in a number of places.
   if (!Lp->getUniqueExitBlock()) {
     reportVectorizationFailure("The loop must have a unique exit block",

llvm/test/Transforms/LoopVectorize/error-handling-remarks.ll

+; RUN: opt -force-vector-width=4 -loop-vectorize -pass-remarks-analysis=loop-vectorize -disable-output < %s 2>&1 | FileCheck %s
+
+; CHECK: remark: error-handling-remarks.cpp:5:5: loop not vectorized: loop exit block contains control flow that does not return
+; CHECK: remark: error-handling-remarks.cpp:13:7: loop not vectorized: loop exit block contains control flow that does not return
+
+define void @f(double* nocapture %X, i64 %N) {
+entry:
+  br label %for.body
+
+for.body:
+  %i = phi i64 [ %inc, %cond.end ], [ 0, %entry ]
+  %arrayidx = getelementptr inbounds double, double* %X, i64 %i
+  %0 = load double, double* %arrayidx, align 8
+  %cmp1 = fcmp une double %0, 0.000000e+00
+  br i1 %cmp1, label %cond.end, label %cond.false
+
+cond.false:
+  call void @__assert_fail(i8* null, i8* null, i32 5, i8* null)
+  unreachable, !dbg !9
+
+cond.end:
+  %div = fdiv double 1.000000e+00, %0
+  store double %div, double* %arrayidx, align 8
+  %inc = add nuw i64 %i, 1
+  %exitcond.not = icmp eq i64 %inc, %N
+  br i1 %exitcond.not, label %for.cond.cleanup, label %for.body
+
+for.cond.cleanup:
+  ret void
+}
+
+define void @g(double* nocapture %X, i64 %N) {
+entry:
+  br  label %for.body
+
+for.body:
+  %i.012 = phi i64 [ %inc, %if.end ], [ 0, %entry ]
+  %arrayidx = getelementptr inbounds double, double* %X, i64 %i.012
+  %0 = load double, double* %arrayidx, align 8
+  %cmp1 = fcmp oeq double %0, 0.000000e+00
+  br i1 %cmp1, label %if.then, label %if.end
+
+if.then:
+  %exception = call i8* @__cxa_allocate_exception(i64 8)
+  %1 = bitcast i8* %exception to i8**
+  store i8* null, i8** %1, align 16
+  call void @__cxa_throw(i8* %exception, i8* null, i8* null)
+  unreachable, !dbg !10
+
+if.end:
+  %div = fdiv double 1.000000e+00, %0
+  store double %div, double* %arrayidx, align 8
+  %inc = add nuw i64 %i.012, 1
+  %exitcond.not = icmp eq i64 %inc, %N
+  br i1 %exitcond.not, label %for.cond.cleanup, label %for.body
+
+for.cond.cleanup:
+  ret void
+}
+
+declare void @__assert_fail(i8*, i8*, i32, i8*)
+
+declare i8* @__cxa_allocate_exception(i64)
+
+declare void @__cxa_throw(i8*, i8*, i8*)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 13.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: NoDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "error-handling-remarks.cpp", directory: "/home2/3n4/llvm/new/llvm-project/build")
+!2 = !{}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{!"clang version 13.0.0"}
+!6 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!7 = distinct !DISubprogram(name: "g", scope: !1, file: !1, line: 10, type: !8, scopeLine: 10, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!8 = !DISubroutineType(types: !2)
+!9 = !DILocation(line: 5, column: 5, scope: !6)
+!10 = !DILocation(line: 13, column: 7, scope: !7)