[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 ↗

(On Diff #200958)

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

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

1435 ↗

(On Diff #200958)

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

7586 ↗

(On Diff #200958)

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 ↗

(On Diff #200958)

Thank you very much! The second message is removed.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

1435 ↗

(On Diff #200958)

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 ↗

(On Diff #200958)

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/trunk/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 a non-int non-pointer 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
         // the PHIs in this block are induction or reduction variables.
         if (BB != Header) {
           // Non-header phi nodes that have outside uses can be vectorized. Add
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/trunk/test/Transforms/LoopVectorize/legal_preheader_check.ll

-; RUN: opt < %s -loop-vectorize -debug -S -o /dev/null 2>&1 | FileCheck %s
-; REQUIRES: asserts
-
-; D40973
-; Make sure LV legal bails out when the loop doesn't have a legal pre-header.
-
-; CHECK: LV: Loop doesn't have a legal pre-header.
-
-define void @inc(i32 %n, i8* %P) {
-  %1 = icmp sgt i32 %n, 0
-  br i1 %1, label %BB1, label %BB2
-
-BB1:
-  indirectbr i8* %P, [label %.lr.ph]
-
-BB2:
-  br label %.lr.ph
-
-.lr.ph:
-  %indvars.iv = phi i32 [ %indvars.iv.next, %.lr.ph ], [ 0, %BB1 ], [ 0, %BB2 ]
-  %indvars.iv.next = add i32 %indvars.iv, 1
-  %exitcond = icmp eq i32 %indvars.iv.next, %n
-  br i1 %exitcond, label %._crit_edge, label %.lr.ph
-
-._crit_edge:
-  ret void
-}

llvm/trunk/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.
+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.
+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
+  br i1 %cmp, label %for.body, label %for.second
+
+for.second:
+  br label %for.body
+}
+
+; Make sure LV legal bails out when there is a non-int, non-ptr phi
+; CHECK-LABEL: "invalid_phi_types"
+; CHECK: LV: Not vectorizing: Found a non-int non-pointer PHI.
+define i32 @invalid_phi_types() {
+entry:
+  br label %for.body
+
+for.body:
+  %i.02 = phi i32 [ 0, %entry ], [ %inc, %for.body ]
+  %vec.sum.02 = phi <2 x i32> [ zeroinitializer, %entry ], [ <i32 8, i32 8>, %for.body ]
+  %inc = add nsw i32 %i.02, 1
+  %cmp = icmp slt i32 %inc, 16
+  br i1 %cmp, label %for.body, label %for.end
+
+for.end:
+  ret i32 0
+}
+
+; D40973
+; Make sure LV legal bails out when the loop doesn't have a legal pre-header.
+; CHECK-LABEL: "inc"
+; CHECK: LV: Not vectorizing: Loop doesn't have a legal pre-header.
+define void @inc(i32 %n, i8* %P) {
+  %1 = icmp sgt i32 %n, 0
+  br i1 %1, label %BB1, label %BB2
+
+BB1:
+  indirectbr i8* %P, [label %.lr.ph]
+
+BB2:
+  br label %.lr.ph
+
+.lr.ph:
+  %indvars.iv = phi i32 [ %indvars.iv.next, %.lr.ph ], [ 0, %BB1 ], [ 0, %BB2 ]
+  %indvars.iv.next = add i32 %indvars.iv, 1
+  %exitcond = icmp eq i32 %indvars.iv.next, %n
+  br i1 %exitcond, label %._crit_edge, label %.lr.ph
+
+._crit_edge:
+  ret void
+}