[LV] Inform about exactly reason of loop illegality

[psamolysov]

Summary

Currently, only the following information is provided by LoopVectorizer
in the case when the CF of the loop is not legal for vectorization:

LV: Can't vectorize the instructions or CFG
   LV: Not vectorizing: Cannot prove legality.

But this information is not enough for the root cause analysis; what is
exactly wrong with the loop should also be printed:

LV: Not vectorizing: The exiting block is not the loop latch.

Also the information which loops were simplified and
eligible for vectorization will be generated when the 'debug-only'
argument is set to 'loop-vectorize'.

[fhahn]

I like the direction of this! Some comments inline. Also, would it be possible to add a test, checking for the expected messages?

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

602

Please unify this debug message with the one at line 606. Having both seems redundant.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

1435

Not sure how helpful this message is. Won't the LV debug output mention the loops it is working on?

7586

Not sure how helpful this information on its own is.

[psamolysov]

As fhahn commented, the message on line 602 repeats one on line 606, remove the last.

[psamolysov]

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

602

Thank you very much! The second message is removed.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

1435

Not all loops (for example, outer loops) can be selected as supported and I think it could be helpful (as least for persons not familiar deeply with the vectorization mechanism) to know which loops are selected to be checked on legality and vectorized and which are not.

7586

I agree, it could be not important which loops were simplified, but simplification can change the CFG and it could be not obvious why the pass is processing not the same IR as it might be expected. But if you or any other reviewers don't need this message, I'll remove it.

[psamolysov]

@fhahn good idea about tge test, I need some time to add it.

[psamolysov]

The tests have also been added to the diff.

[fhahn]

Thanks for adding the test. I think it would be easiest to move this along if you would split off the 2 changes in llvm/lib/Transforms/Vectorize/LoopVectorize.cpp while waiting on additional feedback for those.

llvm/test/Transforms/LoopVectorize/legal_loop_config.ll

1 ↗

(On Diff #201154)

I think -disable-output would be slightly better than -o /dev/null.

[psamolysov]

@fhahn Thank you for the idea, I've reverted the changes in the LoopVectorizer.cpp file and used -disable-output in the tests.

[hsaito]

While we are looking at this, I'd like to discuss how to make these things easier. I think there a merit in using a utility function that takes three strings, something along the lines of
the following pseudo code:

void reportVectorizationFailure(StringRef ORETag, StringRef OREMsg, StringRef DebugMsg) {
      LLVM_DEBUG(dbgs(), concatenate("LV: Not vectorizing: ", DebugMsg));
      ORE->emit(createMissedAnalysis(ORETag), OREMsg);
}

Should reduce/eliminate the chances of missing ORE/DEBUG message (i.e., one exists but not the other) and less boilerplate typing. This is just a direction check. I'm not asking
@psamolysov to do this as part of this patch. Better be a separate NFC patch if pursuing.

[psamolysov]

@hsaito I agree, to have a function to report about an error is a good idea.

I'm new in LLVM community, so what does NFC mean? Should I close this review and open a new one or you mean just to upload a new diff for comments?

[hsaito]

In D62311#1516399, @psamolysov wrote:

@hsaito I agree, to have a function to report about an error is a good idea.

I'm new in LLVM community, so what does NFC mean? Should I close this review and open a new one or you mean just to upload a new diff for comments?

NFC stands for Non Functional Change. No, your review is good here. I'm just throwing an idea for another separate patch, trying to get other people's opinion,
while I was thinking why LLVM_DEBUG wasn't added here when ORE->emit was added.

[psamolysov]

@hsaito @fhahn I've introduced the 'reportVectorizationFailure' function, a NFC patch is open for review: D62478 This patch stands unchanged and if you accept it, ready to be applied.

[fhahn]

LGTM. Please take a look at the remaining test nits before committing

llvm/test/Transforms/LoopVectorize/legal_loop_config.ll

1 ↗

(On Diff #201196)

nit: Maybe something like loop-legality-checks.ll would be a slightly more descriptive name.

llvm/test/Transforms/LoopVectorize/legal_preheader_check.ll

1 ↗

(On Diff #201196)

maybe fold this test into llvm/test/Transforms/LoopVectorize/legal_loop_config.ll, it would make sense to have the legality checks in a test file together.

[psamolysov]

@fhahn thank you! I've updated the diff - consolidate the legality checking tests in one file: loop-legality-checks.ll
I have no commit access, so could you or someone else lend the patch?

[fhahn]

Will do, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

bool LoopVectorizationLegality::canVectorizeInstrs() {
   for (BasicBlock *BB : TheLoop->blocks()) {
     // Scan the instructions in the block and look for hazards.
     for (Instruction &I : *BB) {
       if (auto *Phi = dyn_cast<PHINode>(&I)) {
         Type *PhiTy = Phi->getType();
         // Check that this PHI type is allowed.
         if (!PhiTy->isIntegerTy() && !PhiTy->isFloatingPointTy() &&
             !PhiTy->isPointerTy()) {
+          LLVM_DEBUG(dbgs()
+                 << "LV: Not vectorizing: Found an unsupported type of PHI.\n");
           ORE->emit(createMissedAnalysis("CFGNotUnderstood", Phi)
                     << "loop control flow is not understood by vectorizer");
           LLVM_DEBUG(dbgs() << "LV: Found an non-int non-pointer PHI.\n");
           return false;
         }
 
         // If this PHINode is not in the header block, then we know that we
         // can convert it to select during if-conversion. No need to check if
bool LoopVectorizationLegality::canVectorizeLoopCFG(Loop *Lp,
   // Store the result and return it at the end instead of exiting early, in case
   // allowExtraAnalysis is used to report multiple reasons for not vectorizing.
   bool Result = true;
   bool DoExtraAnalysis = ORE->allowExtraAnalysis(DEBUG_TYPE);
 
   // We must have a loop in canonical form. Loops with indirectbr in them cannot
   // be canonicalized.
   if (!Lp->getLoopPreheader()) {
-    LLVM_DEBUG(dbgs() << "LV: Loop doesn't have a legal pre-header.\n");
+    LLVM_DEBUG(dbgs()
+             << "LV: Not vectorizing: Loop doesn't have a legal pre-header.\n");
     ORE->emit(createMissedAnalysis("CFGNotUnderstood")
               << "loop control flow is not understood by vectorizer");
     if (DoExtraAnalysis)
       Result = false;
     else
       return false;
   }
 
   // We must have a single backedge.
   if (Lp->getNumBackEdges() != 1) {
+    LLVM_DEBUG(dbgs()
+            << "LV: Not vectorizing: The loop must have a single backedge.\n");
     ORE->emit(createMissedAnalysis("CFGNotUnderstood")
               << "loop control flow is not understood by vectorizer");
     if (DoExtraAnalysis)
       Result = false;
     else
       return false;
   }
 
   // We must have a single exiting block.
   if (!Lp->getExitingBlock()) {
+    LLVM_DEBUG(dbgs()
+            << "LV: Not vectorizing: The loop must have an exiting block.\n");
     ORE->emit(createMissedAnalysis("CFGNotUnderstood")
               << "loop control flow is not understood by vectorizer");
     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()) {
+    LLVM_DEBUG(dbgs()
+          << "LV: Not vectorizing: The exiting block is not the loop latch.\n");
     ORE->emit(createMissedAnalysis("CFGNotUnderstood")
               << "loop control flow is not understood by vectorizer");
     if (DoExtraAnalysis)
       Result = false;
     else
       return false;
   }
 

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

static void collectSupportedLoops(Loop &L, LoopInfo *LI,
   // are stress testing the VPlan H-CFG construction, we collect the outermost
   // loop of every loop nest.
   if (L.empty() || VPlanBuildStressTest ||
       (EnableVPlanNativePath && isExplicitVecOuterLoop(&L, ORE))) {
     LoopBlocksRPO RPOT(&L);
     RPOT.perform(LI);
     if (!containsIrreducibleCFG<const BasicBlock *>(RPOT, *LI)) {
       V.push_back(&L);
+      LLVM_DEBUG(dbgs() << "LV: The loop '"
+                        << L.getName()
+                        << "' is eligible as supported.\n");
       // TODO: Collect inner loops inside marked outer loops in case
       // vectorization fails for the outer loop. Do not invoke
       // 'containsIrreducibleCFG' again for inner loops when the outer loop is
       // already known to be reducible. We can use an inherited attribute for
       // that.
       return;
     }
   }
bool LoopVectorizePass::runImpl(
 
   bool Changed = false;
 
   // The vectorizer requires loops to be in simplified form.
   // Since simplification may add new inner loops, it has to run before the
   // legality and profitability checks. This means running the loop vectorizer
   // will simplify all loops, regardless of whether anything end up being
   // vectorized.
-  for (auto &L : *LI)
-    Changed |=
+  for (auto &L : *LI) {
+    bool Simplified =
         simplifyLoop(L, DT, LI, SE, AC, nullptr, false /* PreserveLCSSA */);
+    LLVM_DEBUG(
+      if (Simplified)
+        dbgs() << "LV: The loop '" << L->getName() << "' has been simplified.\n";
+    );
+    Changed |= Simplified;
+  }
 
   // Build up a worklist of inner-loops to vectorize. This is necessary as
   // the act of vectorizing or partially unrolling a loop creates new loops
   // and can invalidate iterators across the loops.
   SmallVector<Loop *, 8> Worklist;
 
-  for (Loop *L : *LI)
+  for (auto &L : *LI)
     collectSupportedLoops(*L, LI, ORE, Worklist);
 
   LoopsAnalyzed += Worklist.size();
 
   // Now walk the identified inner loops.
   while (!Worklist.empty()) {
     Loop *L = Worklist.pop_back_val();