[Loop-Vectorize] Emit more context in remarks for optimization record This extends the reportVectorizationFailure helper function with more information emitted after setExtraArgs. This avoids cluttering the compiler frontend remarks with low...

[hnrklssn]

Summary

...-level details, while still allowing advanced users to get more context without activating debug output.
There are currently many different cases that result in identical remarks about the vectorizer not understanding the loop control flow, and this change aims to disambiguate those cases.

[hnrklssn]

Updating D79564: [Loop-Vectorize] update test cases to accept new ouput

This extends the reportVectorizationFailure helper function with more information emitted after setExtraArgs. This avoids cluttering the compiler frontend remarks with low...

[fhahn]

Thanks for working on improving this!

Could you add some tests to check the additional context?

One thing we have to be a bit careful about is to not provide too much information tied to LLVM IR, as this can be confusing to users when viewing the remarks in the original source (which has no references to the IR). IIUC you are using allowExtraAnalysis as a proxy for whether the IR info should be included. Not sure what the current practice is in other passes, @anemet or @thegameg might have some input there.

An alternative might be to use the debug info to reference the original location in the source language. But there are also scenarios where this might fall apart (e..g if the problematic exits have been added by other passes).

llvm/include/llvm/Transforms/Vectorize/LoopVectorize.h

179

could this be llvm::function_ref?

also, might be good to rename ExtraInfo to something like GetExtraInfo, to have the name indicate that this is a function.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

1087

things are quite deeply nested here. It might be better to flip the logic in the checks and have

if (T->getSuccessor(i) != Head)
  continue;

Same for T->getNumSuccessors.

Also if you are only interested in iteration over the successor blocks, you could use successors(Latch).

1089

nit: StringREf?

1091

nit: unnecessary {}

1147

Because of the check at line 1137, shouldn't we only reach here if both the latch and exiting block are != nullptr, i.e. wouldn't the exiting block always be used here?

[fhahn]

Marking as requiring changes, to remove from review-list until updated.

llvm/include/llvm/Transforms/Vectorize/LoopVectorize.h

 //  Vectorizing Compilers.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TRANSFORMS_VECTORIZE_LOOPVECTORIZE_H
 #define LLVM_TRANSFORMS_VECTORIZE_LOOPVECTORIZE_H
 
 #include "llvm/Analysis/AliasAnalysis.h"
+#include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Support/CommandLine.h"
 #include <functional>
 
 namespace llvm {
 
 class AssumptionCache;
 class BlockFrequencyInfo;
public:
 
   bool processLoop(Loop *L);
 };
 
 /// Reports a vectorization failure: print \p DebugMsg for debugging
 /// purposes along with the corresponding optimization remark \p RemarkName.
 /// If \p I is passed, it is an instruction that prevents vectorization.
 /// Otherwise, the loop \p TheLoop is used for the location of the remark.
-void reportVectorizationFailure(const StringRef DebugMsg,
-    const StringRef OREMsg, const StringRef ORETag,
-    OptimizationRemarkEmitter *ORE, Loop *TheLoop, Instruction *I = nullptr);
+/// Optionally, a function can be passed to add extra arguments for further
+/// explanation in optimization reports.
+void reportVectorizationFailure(
+    const StringRef DebugMsg, const StringRef OREMsg, const StringRef ORETag,
+    OptimizationRemarkEmitter *ORE, Loop *TheLoop, Instruction *I = nullptr,
+    std::function<void(OptimizationRemarkAnalysis &)> ExtraInfo = nullptr);
 
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_VECTORIZE_LOOPVECTORIZE_H

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

 // resided in LoopVectorize.cpp for a long time.
 //
 // At this point, it is implemented as a utility class, not as an analysis
 // pass. It should be easy to create an analysis pass around it if there
 // is a need (but D45420 needs to happen first).
 //
 #include "llvm/Transforms/Vectorize/LoopVectorizationLegality.h"
 #include "llvm/Analysis/Loads.h"
+#include "llvm/Analysis/OptimizationRemarkEmitter.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/Analysis/VectorUtils.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/Transforms/Vectorize/LoopVectorize.h"
+#include <functional>
+#include <string>
 
 using namespace llvm;
 using namespace PatternMatch;
 
 #define LV_NAME "loop-vectorize"
 #define DEBUG_TYPE LV_NAME
 
 extern cl::opt<bool> EnableVPlanPredication;
if (!Lp->getLoopPreheader()) {
     if (DoExtraAnalysis)
       Result = false;
     else
       return false;
   }
 
   // We must have a single backedge.
   if (Lp->getNumBackEdges() != 1) {
-    reportVectorizationFailure("The loop must have a single backedge",
-        "loop control flow is not understood by vectorizer",
-        "CFGNotUnderstood", ORE, TheLoop);
+    auto ExtraInfoCb = [&](OptimizationRemarkAnalysis &Analysis) {
+      SmallVector<BasicBlock *, 2> LoopLatches;
+      auto Head = Lp->getHeader();
+      Lp->getLoopLatches(LoopLatches);
+      for (auto Latch : LoopLatches) {
+        auto T = Latch->getTerminator();
+        for (unsigned i = 0; i < T->getNumSuccessors(); i++) {
+          if (T->getSuccessor(i) == Head) {
+            if (T->getNumSuccessors() > 1) {
+              std::string Description = "backedge (condition ";
+              Analysis << ore::NV(Description + (i ? "false)" : "true)"), T);
+            } else {
+              Analysis << ore::NV("backedge", T);
+            }
+          }
+        }
+      }
+    };
+    reportVectorizationFailure(
+        "The loop must have a single backedge",
+        "loop control flow is not understood by vectorizer", "CFGNotUnderstood",
+        ORE, TheLoop, nullptr, ExtraInfoCb);
     if (DoExtraAnalysis)
       Result = false;
     else
       return false;
   }
 
   // We must have a single exiting block.
   if (!Lp->getExitingBlock()) {
-    reportVectorizationFailure("The loop must have an exiting block",
-        "loop control flow is not understood by vectorizer",
-        "CFGNotUnderstood", ORE, TheLoop);
+    auto ExtraInfoCb = [&](OptimizationRemarkAnalysis &Analysis) {
+      SmallVector<BasicBlock *, 4> Exits;
+      Lp->getExitingBlocks(Exits);
+      if (Exits.empty()) {
+        Analysis << ". There is no exiting block.";
+        return;
+      }
+      Analysis << ". Exiting blocks: ";
+      for (auto Exit : Exits) {
+        Analysis << ore::NV("exiting block terminator", Exit->getTerminator());
+      }
+    };
+    reportVectorizationFailure(
+        "The loop must have a single exiting block",
+        "loop control flow is not understood by vectorizer", "CFGNotUnderstood",
+        ORE, TheLoop, nullptr, ExtraInfoCb);
     if (DoExtraAnalysis)
       Result = false;
     else
       return false;
   }
 
   // We only handle bottom-tested loops, i.e. loop in which the condition is
   // checked at the end of each iteration. With that we can assume that all
   // instructions in the loop are executed the same number of times.
   if (Lp->getExitingBlock() != Lp->getLoopLatch()) {
-    reportVectorizationFailure("The exiting block is not the loop latch",
-        "loop control flow is not understood by vectorizer",
-        "CFGNotUnderstood", ORE, TheLoop);
+    auto ExtraInfoCb = [&](OptimizationRemarkAnalysis &Analysis) {
+      if (!Lp->getExitingBlock() || !Lp->getLoopLatch())
+        return;
+      Analysis << ore::NV("exiting block terminator",
+                          Lp->getExitingBlock()->getTerminator())
+               << ore::NV("loop latch terminator",
+                          Lp->getLoopLatch()->getTerminator());
+    };
+    Instruction *Term = nullptr;
+    // This will highlight one of the relevant BBs in case extra info is not
+    // included
+    if (Lp->getExitingBlock())
+      Term = Lp->getExitingBlock()->getTerminator();
+    else if (Lp->getLoopLatch())
+      Term = Lp->getLoopLatch()->getTerminator();
+    reportVectorizationFailure(
+        "The exiting block is not the loop latch",
+        "loop control flow is not understood by vectorizer", "CFGNotUnderstood",
+        ORE, TheLoop, Term, ExtraInfoCb);
     if (DoExtraAnalysis)
       Result = false;
     else
       return false;
   }
 
   return Result;
 }

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

static OptimizationRemarkAnalysis createLVAnalysis(const char *PassName,
 
   OptimizationRemarkAnalysis R(PassName, RemarkName, DL, CodeRegion);
   R << "loop not vectorized: ";
   return R;
 }
 
 namespace llvm {
 
-void reportVectorizationFailure(const StringRef DebugMsg,
-    const StringRef OREMsg, const StringRef ORETag,
-    OptimizationRemarkEmitter *ORE, Loop *TheLoop, Instruction *I) {
+void reportVectorizationFailure(
+    const StringRef DebugMsg, const StringRef OREMsg, const StringRef ORETag,
+    OptimizationRemarkEmitter *ORE, Loop *TheLoop, Instruction *I,
+    std::function<void(OptimizationRemarkAnalysis &)> ExtraInfo) {
   LLVM_DEBUG(debugVectorizationFailure(DebugMsg, I));
   LoopVectorizeHints Hints(TheLoop, true /* doesn't matter */, *ORE);
-  ORE->emit(createLVAnalysis(Hints.vectorizeAnalysisPassName(),
-                ORETag, TheLoop, I) << OREMsg);
+  auto Analysis =
+      createLVAnalysis(Hints.vectorizeAnalysisPassName(), ORETag, TheLoop, I)
+      << OREMsg;
+  Analysis << ore::setExtraArgs() << ore::NV("DebugMsg", DebugMsg);
+  if (ORE->allowExtraAnalysis(DEBUG_TYPE) && ExtraInfo) {
+    ExtraInfo(Analysis);
+  }
+  ORE->emit(Analysis);
 }
 
 } // end namespace llvm
 
 #ifndef NDEBUG
 /// \return string containing a file name and a line # for the given loop.
 static std::string getDebugLocString(const Loop *L) {
   std::string Result;

llvm/test/Transforms/LoopVectorize/X86/vectorization-remarks-missed.ll

 ;     if (A[i])
 ;       k = foo();
 ;   }
 ;   return k;
 ; }
 ; CHECK: remark: source.cpp:29:7: loop not vectorized: control flow cannot be substituted for a select
 ; CHECK: remark: source.cpp:27:3: loop not vectorized
 
-; YAML:       --- !Analysis
+; YAML:      --- !Analysis
 ; YAML-NEXT: Pass:            loop-vectorize
 ; YAML-NEXT: Name:            CantComputeNumberOfIterations
 ; YAML-NEXT: DebugLoc:        { File: source.cpp, Line: 5, Column: 9 }
 ; YAML-NEXT: Function:        _Z4testPii
 ; YAML-NEXT: Args:
 ; YAML-NEXT:   - String:          'loop not vectorized: '
 ; YAML-NEXT:   - String:          could not determine number of loop iterations
 ; YAML-NEXT: ...
 ; YAML-NEXT: --- !Missed
 ; YAML-NEXT: Pass:            loop-vectorize
 ; YAML-NEXT: Name:            MissedDetails
 ; YAML-NEXT: DebugLoc:        { File: source.cpp, Line: 5, Column: 9 }
 ; YAML-NEXT: Function:        _Z4testPii
 ; YAML-NEXT: Args:
 ; YAML-NEXT:   - String:          loop not vectorized
 ; YAML-NEXT: ...
 ; YAML-NEXT: --- !Analysis
 ; YAML-NEXT: Pass:            loop-vectorize
 ; YAML-NEXT: Name:            AllDisabled
 ; YAML-NEXT: DebugLoc:        { File: source.cpp, Line: 12, Column: 8 }
 ; YAML-NEXT: Function:        _Z13test_disabledPii
 ; YAML-NEXT: Args:
-; YAML-NEXT:   - String:          'loop not vectorized: vectorization and interleaving are explicitly disabled, or the loop has already been vectorized
+; YAML-NEXT:   - String:          'loop not vectorized: vectorization and interleaving are explicitly disabled, or the loop has already been vectorized'
 ; YAML-NEXT: ...
 ; YAML-NEXT: --- !Analysis
 ; YAML-NEXT: Pass:            ''
 ; YAML-NEXT: Name:            CantIdentifyArrayBounds
 ; YAML-NEXT: DebugLoc:        { File: source.cpp, Line: 19, Column: 5 }
 ; YAML-NEXT: Function:        _Z17test_array_boundsPiS_i
 ; YAML-NEXT: Args:
 ; YAML-NEXT:   - String:          'loop not vectorized: '

 ; YAML-NEXT: --- !Analysis
 ; YAML-NEXT: Pass:            loop-vectorize
 ; YAML-NEXT: Name:            NoCFGForSelect
 ; YAML-NEXT: DebugLoc:        { File: source.cpp, Line: 29, Column: 7 }
 ; YAML-NEXT: Function:        test_multiple_failures
 ; YAML-NEXT: Args:
 ; YAML-NEXT:   - String:          'loop not vectorized: '
 ; YAML-NEXT:   - String:          control flow cannot be substituted for a select
+; YAML-NEXT:   - DebugMsg:        Control flow cannot be substituted for a select
 ; YAML-NEXT: ...
 ; YAML-NEXT: --- !Analysis
 ; YAML-NEXT: Pass:            loop-vectorize
 ; YAML-NEXT: Name:            NonReductionValueUsedOutsideLoop
 ; YAML-NEXT: DebugLoc:        { File: source.cpp, Line: 27, Column: 3 }
 ; YAML-NEXT: Function:        test_multiple_failures
 ; YAML-NEXT: Args:
 ; YAML-NEXT:   - String:          'loop not vectorized: '
 ; YAML-NEXT:   - String:          value that could not be identified as reduction is used outside the loop
+; YAML-NEXT:   - DebugMsg:        Found an unidentified PHI
 ; YAML-NEXT: ...
 ; YAML-NEXT: --- !Analysis
 ; YAML-NEXT: Pass:            loop-vectorize
 ; YAML-NEXT: Name:            CantComputeNumberOfIterations
 ; YAML-NEXT: DebugLoc:        { File: source.cpp, Line: 27, Column: 3 }
 ; YAML-NEXT: Function:        test_multiple_failures
 ; YAML-NEXT: Args:
 ; YAML-NEXT:   - String:          'loop not vectorized: '

llvm/test/Transforms/LoopVectorize/loop-legality-checks.ll

 ; RUN: opt < %s -loop-vectorize -debug-only=loop-vectorize -S -disable-output 2>&1 | FileCheck %s
 ; REQUIRES: asserts
 
 ; Make sure LV legal bails out when the exiting block != loop latch.
 ; CHECK-LABEL: "latch_is_not_exiting"
-; CHECK: LV: Not vectorizing: The exiting block is not the loop latch.
+; CHECK: LV: Not vectorizing: The exiting block is not the loop latch br i1 %or.cond, label %for.body.backedge, label %for.end
 define i32 @latch_is_not_exiting() {
 entry:
   br label %for.body
 
 for.body:
   %i.02 = phi i32 [ 0, %entry ], [ %inc, %for.body ], [%inc, %for.second]
   %inc = add nsw i32 %i.02, 1
   %cmp = icmp slt i32 %inc, 16
   br i1 %cmp, label %for.body, label %for.second
 
 for.second:
   %cmps = icmp sgt i32 %inc, 16
   br i1 %cmps, label %for.body, label %for.end
 
 for.end:
   ret i32 0
 }
 
 ; Make sure LV legal bails out when there is no exiting block
 ; CHECK-LABEL: "no_exiting_block"
-; CHECK: LV: Not vectorizing: The loop must have an exiting block.
+; CHECK: LV: Not vectorizing: The loop must have a single exiting block.
 define i32 @no_exiting_block() {
 entry:
   br label %for.body
 
 for.body:
   %i.02 = phi i32 [ 0, %entry ], [ %inc, %for.body ], [%inc, %for.second]
   %inc = add nsw i32 %i.02, 1
   %cmp = icmp slt i32 %inc, 16