[LV] Remove unnecessary DoExtraAnalysis guard (silent bug)

[dcaballe]

Summary

canVectorize is only checking if the loop has a normalized pre-header if DoExtraAnalysis is true.
This doesn't make sense to me because reporting analysis information shouldn't alter legality
checks. This is probably the result of a last minute minor change before committing (?).

Patch by Diego Caballero.

[fhahn]

This seems like a bug indeed! Could you add a test case without a preheader, to make sure we catch this regression in the future?

lib/Transforms/Vectorize/LoopVectorize.cpp

5000 ↗

(On Diff #125997)

unrelated whitespace change?

5008 ↗

(On Diff #125997)

Could you check if this has the correct indentation? It looks off in the Phabricator web view.

[dcaballe]

Thanks for the quick response, Florian!

I had a test but it didn't convince me. In order to test that a loop without PH is hitting this check we would have to match the ORE message:

ORE->emit(createMissedAnalysis("CFGNotUnderstood")
          << "loop control flow is not understood by vectorizer");

In order to emit this message, we have to use -pass-remarks-analysis=loop-vectorize, which is setting DoExtraAnalysis to 'true'. This means that the test will pass with and without this fix :)
In addition, the message "loop control flow is not understood by vectorizer" is reused 4 more times in other legal checks so the test will pass if that message is printed by a check not related to the loop PH.

However, I can add the test I have if you still think it has some value. IMO, I don't think so.
If somebody knows a more reliable way to test this, please, let me know. Of course, the right thing to do would be to replace "loop control flow is not understood by vectorizer" messages with more specific messages depending on the legal check but that is out of the scope of this fix and maybe this message was reused for a good reason.

I will wait for your response to commit the last version of the patch with or without the test.

Thanks,
Diego

lib/Transforms/Vectorize/LoopVectorize.cpp

5008 ↗

(On Diff #125997)

No, the indentation was wrong. I fixed it as part of this patch because it's related to the issue. However, I could commit it separately if there is any problem with that.

[fhahn]

In D40973#948940, @dcaballe wrote:

Thanks for the quick response, Florian!

I had a test but it didn't convince me. In order to test that a loop without PH is hitting this check we would have to match the ORE message:

Without the early exit, we should fail later in the vectorizer. There is some debug output we could use and check if it does not get emitted when the early exit gets taken?

For example, for the following test function, without your change, the vectorizer bails out because the PHI node contains 3 entries.

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
}

[dcaballe]

Add test + new debug message for pre-header check.

[dcaballe]

Thanks for the test, Florian.

Maybe I'm missing something but the bug/fix don't affect the early exit flag value (DoExtraAnalysis) so the early exit behavior won't change before/after the fix.
Instead of matching debug info that is not related to the pre-header check (and may lead to unrelated failures in the future), I thought that we could add a new debug message for the pre-header check.
It seems a more reliable way to test it.

What do you think? Is my latest diff reasonable?

Diego

[fhahn]

LGTM thanks. I think this should be a NFC as to vectorization, as without pre-header at least one PHI node in the loop should have at least 3 entries (2+ for the loop predecessors and one backedge).

[dcaballe]

Thanks, Florian. Could someone commit this patch for me? I don't have commit access yet.

Thanks,
Diego

[fhahn]

Done! Thanks for the patch

[dcaballe]

Great! Thanks, Florian!

llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp

 }
 
 bool LoopVectorizationLegality::canVectorize() {
   // 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);
-  if (DoExtraAnalysis)
   // We must have a loop in canonical form. Loops with indirectbr in them cannot
   // be canonicalized.
   if (!TheLoop->getLoopPreheader()) {
+    DEBUG(dbgs() << "LV: 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;
   }
 
   // FIXME: The code is currently dead, since the loop gets sent to
   // LoopVectorizationLegality is already an innermost loop.
   //

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
+}