[LV] Fix emission of debug message in legality check #101924

madhur13490 · 2024-08-05T06:19:34Z

Successful vectorization message is emitted even
after "Result" is false. "Result" = false indicates failure of one of the legality check and thus
successful message should not be printed.

llvmbot · 2024-08-05T06:20:05Z

@llvm/pr-subscribers-llvm-transforms

Author: Madhur Amilkanthwar (madhur13490)

Changes

Successful vectorization message is emitted even
after "Result" is false. "Result" = false indicates failure of one of the legality check and thus
successful message should not be printed.

Full diff: https://github.com/llvm/llvm-project/pull/101924.diff

1 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp (+11-7)

diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
index 465d0df30e3f7..35c841f470261 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
@@ -1441,10 +1441,12 @@ bool LoopVectorizationLegality::canVectorize(bool UseVPlanNativePath) {
   // Check whether the loop-related control flow in the loop nest is expected by
   // vectorizer.
   if (!canVectorizeLoopNestCFG(TheLoop, UseVPlanNativePath)) {
-    if (DoExtraAnalysis)
+    if (DoExtraAnalysis) {
+      LLVM_DEBUG(dbgs() << "LV legality check failed: loop nest");
       Result = false;
-    else
+    } else {
       return false;
+    }
   }
 
   // We need to have a loop header.
@@ -1509,11 +1511,6 @@ bool LoopVectorizationLegality::canVectorize(bool UseVPlanNativePath) {
       return false;
   }
 
-  LLVM_DEBUG(dbgs() << "LV: We can vectorize this loop"
-                    << (LAI->getRuntimePointerChecking()->Need
-                            ? " (with a runtime bound check)"
-                            : "")
-                    << "!\n");
 
   unsigned SCEVThreshold = VectorizeSCEVCheckThreshold;
   if (Hints->getForce() == LoopVectorizeHints::FK_Enabled)
@@ -1528,6 +1525,13 @@ bool LoopVectorizationLegality::canVectorize(bool UseVPlanNativePath) {
     else
       return false;
   }
+  if (Result) {
+    LLVM_DEBUG(dbgs() << "LV: We can vectorize this loop"
+                      << (LAI->getRuntimePointerChecking()->Need
+                            ? " (with a runtime bound check)"
+                            : "")
+                      << "!\n");
+  }
 
   // Okay! We've done all the tests. If any have failed, return false. Otherwise
   // we can vectorize, and at this point we don't have any other mem analysis

github-actions · 2024-08-05T06:23:38Z

✅ With the latest revision this PR passed the C/C++ code formatter.

artagnon · 2024-08-07T11:24:50Z

Kindly add a test?

madhur13490 · 2024-08-10T05:56:21Z

Kindly add a test?

Done. The test makes sure that we don't emit both successful and unsuccessful message about vectorization.

madhur13490 · 2024-08-20T08:45:44Z

Gentle ping to the reviewers!

nikic · 2024-08-20T08:49:44Z

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

@@ -1528,6 +1525,13 @@ bool LoopVectorizationLegality::canVectorize(bool UseVPlanNativePath) {
    else
      return false;
  }


nit: I'd add an empty line between these.

Refreshed code.

nikic · 2024-08-20T08:50:17Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+; successful and unsuccessful message about vectorization.
+
+; RUN: opt -passes=loop-vectorize -debug -disable-output -pass-remarks-missed=loop-vectorize %s 2>&1 | FileCheck %s
+; CHECK-NOT: LV: We can vectorize this loop


I'd also add this CHECK-NOT after the CHECK as well, to make sure it doesn't happen in either order.

nikic · 2024-08-20T08:50:52Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+; CHECK: LV: Not vectorizing: Cannot prove legality
+
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128-Fn32"
+target triple = "aarch64-unknown-linux-gnu"


Does this test depend on the target triple? If so, it must go in the AArch64 directory. Though ideally it would be triple-independent.

No, this does not depend on TT. I have done the change.

nikic · 2024-08-20T08:51:11Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+@a = dso_local global [32000 x i32] zeroinitializer, align 4
+@b = dso_local global [32000 x i32] zeroinitializer, align 4
+
+define dso_local void @foo() local_unnamed_addr {


Unnecessary dso_local / local_unnamed_addr.

Thanks, done!

fhahn · 2024-08-20T13:35:48Z

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

@@ -1528,6 +1525,13 @@ bool LoopVectorizationLegality::canVectorize(bool UseVPlanNativePath) {
    else
      return false;
  }
+  if (Result) {
+    LLVM_DEBUG(dbgs() << "LV: We can vectorize this loop"


Can this be kept at the original place? the check above is a profitability check, we can still vectorize the loop

Might be good to have a test case

Done. But I think the meaning attached to "false" returned from this function is slightly misleading. If "false" is returned through the last instance then the loop is vectorizable but not profitable. For other instances, it is not vectorizable at all. I think the name of the function should be canVectorizeAndProfitable.

artagnon · 2024-08-22T21:46:28Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+  ret void
+
+for.body:                                         ; preds = %entry, %for.body
+  %0 = phi i32 [ %.pre17, %entry ], [ %add6, %for.body ]


Would be nice to give the variables better names: %0, %1, %.pre, %.pre17 etc.

artagnon · 2024-08-22T21:46:47Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+; CHECK: LV: Not vectorizing: Cannot prove legality
+; CHECK-NOT: LV: We can vectorize this loop
+
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128-Fn32"


Is this necessary?

Not really, removed.

artagnon · 2024-08-22T21:47:43Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+; This test asserts that we don't emit both
+; successful and unsuccessful message about vectorization.
+
+; RUN: opt -passes=loop-vectorize -debug -disable-output -pass-remarks-missed=loop-vectorize %s 2>&1 | FileCheck %s


-debug-only=loop-vectorize? Are we checking pass remarks as well?

Yes, thanks for catching!

artagnon · 2024-08-22T21:49:15Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

@@ -0,0 +1,36 @@
+; This test asserts that we don't emit both


Nit: I'd probably name this test dbg-print-no-vectorize.

I think the name is fine.

Reflow comment

Suggested change

; This test asserts that we don't emit both

; This test checks that we don't emit both successful and unsuccessful

artagnon · 2024-08-22T21:50:36Z

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

@@ -1441,10 +1441,12 @@ bool LoopVectorizationLegality::canVectorize(bool UseVPlanNativePath) {
  // Check whether the loop-related control flow in the loop nest is expected by
  // vectorizer.
  if (!canVectorizeLoopNestCFG(TheLoop, UseVPlanNativePath)) {
-    if (DoExtraAnalysis)
+    if (DoExtraAnalysis) {
+      LLVM_DEBUG(dbgs() << "LV legality check failed: loop nest");


Perhaps possible to check this message as well? Shouldn't the debug message start with an "LV: " prefix for uniformity?

artagnon · 2024-08-22T22:57:08Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+; This test asserts that we don't emit both
+; successful and unsuccessful message about vectorization.
+
+; RUN: opt -passes=loop-vectorize -debug -disable-output -pass-remarks-missed=loop-vectorize %s 2>&1 | FileCheck %s


Needs REQUIRES: asserts, since we're checking debug printing.

fhahn · 2024-08-27T15:29:38Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+for.body:                                         ; preds = %entry, %for.body
+  %0 = phi i32 [ %val_a, %entry ], [ %add6, %for.body ]
+  %1 = phi i32 [ %load_a_gep, %entry ], [ %2, %for.body ]
+  %indvars.iv = phi i64 [ 1, %entry ], [ %indvars.iv.next, %for.body ]


Suggested change

%indvars.iv = phi i64 [ 1, %entry ], [ %indvars.iv.next, %for.body ]

%iv = phi i64 [ 1, %entry ], [ %indvars.iv.next, %for.body ]

fhahn · 2024-08-27T15:29:48Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+  %1 = phi i32 [ %load_a_gep, %entry ], [ %2, %for.body ]
+  %indvars.iv = phi i64 [ 1, %entry ], [ %indvars.iv.next, %for.body ]
+  %arrayidx = getelementptr inbounds [32000 x i32], ptr @a, i64 0, i64 %indvars.iv
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1


Suggested change

%indvars.iv.next = add nuw nsw i64 %indvars.iv, 1

%iv.next = add nuw nsw i64 %indvars.iv, 1

fhahn · 2024-08-27T15:30:06Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+
+define void @foo() {
+entry:
+  %load_a_gep = load i32, ptr getelementptr inbounds (i8, ptr @a, i64 4), align 4


pass as arg to simplify test?

fhahn · 2024-08-27T15:30:10Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+define void @foo() {
+entry:
+  %load_a_gep = load i32, ptr getelementptr inbounds (i8, ptr @a, i64 4), align 4
+  %val_a = load i32, ptr @a, align 4


pass as arg to simplify test?

Ok, sure. Done

fhahn · 2024-08-27T15:30:25Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+; CHECK-NOT: LV: We can vectorize this loop
+
+@a = global [32000 x i32] zeroinitializer, align 4
+@b = global [32000 x i32] zeroinitializer, align 4


Ah! yes, removed.

fhahn · 2024-08-27T15:30:57Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

+  %val_a = load i32, ptr @a, align 4
+  br label %for.body
+
+for.cond.cleanup:                                 ; preds = %for.body


might be good to move this to the end of the function and shorten name to something like exit

fhahn · 2024-08-27T15:31:54Z

llvm/test/Transforms/LoopVectorize/check-no-vectorize.ll

@@ -0,0 +1,36 @@
+; This test asserts that we don't emit both


Reflow comment

Suggested change

; This test asserts that we don't emit both

; This test checks that we don't emit both successful and unsuccessful

fhahn · 2024-08-27T15:32:16Z

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

@@ -1451,10 +1451,12 @@ bool LoopVectorizationLegality::canVectorize(bool UseVPlanNativePath) {
  // Check whether the loop-related control flow in the loop nest is expected by
  // vectorizer.
  if (!canVectorizeLoopNestCFG(TheLoop, UseVPlanNativePath)) {
-    if (DoExtraAnalysis)
+    if (DoExtraAnalysis) {
+      LLVM_DEBUG(dbgs() << "LV: legality check failed: loop nest");


unrelated to the original change?

Yes, but doing it as good to have a debug message.

Ok, generally it's preferable to keep things separate if possible. Fine to keep it here in the PR for now though

Sure, thanks!

madhur13490 · 2024-09-03T16:36:10Z

@fhahn Does the new revision look ok now?

fhahn · 2024-09-03T19:14:30Z

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

@@ -1451,10 +1451,12 @@ bool LoopVectorizationLegality::canVectorize(bool UseVPlanNativePath) {
  // Check whether the loop-related control flow in the loop nest is expected by
  // vectorizer.
  if (!canVectorizeLoopNestCFG(TheLoop, UseVPlanNativePath)) {
-    if (DoExtraAnalysis)
+    if (DoExtraAnalysis) {
+      LLVM_DEBUG(dbgs() << "LV: legality check failed: loop nest");


Ok, generally it's preferable to keep things separate if possible. Fine to keep it here in the PR for now though

fhahn · 2024-09-03T19:15:37Z

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

  unsigned SCEVThreshold = VectorizeSCEVCheckThreshold;
  if (Hints->getForce() == LoopVectorizeHints::FK_Enabled)
    SCEVThreshold = PragmaVectorizeSCEVCheckThreshold;

+  if (Result) {


also keep where the message is emitted at the original place, so SCEVThreshold's definition is closer to use?

Oh yes, there is no reason to change. Thanks! Hopefully, looks good now :)

fhahn

LGTM, thanks!

Successful vectorization message is emitted even after "Result" is false. "Result" = false indicates failure of one of the legality check and thus successful message should not be printed.

llvmbot added vectorizers llvm:transforms labels Aug 5, 2024

madhur13490 requested a review from fhahn August 5, 2024 06:20

madhur13490 requested a review from davemgreen August 5, 2024 06:20

madhur13490 force-pushed the madhur13490/correct-lv-info branch from 6782629 to 444a13a Compare August 5, 2024 06:29

madhur13490 force-pushed the madhur13490/correct-lv-info branch from 444a13a to d5faf52 Compare August 10, 2024 05:54

madhur13490 requested review from nikic, alexey-bataev, sdesmalen-arm and sjoerdmeijer August 12, 2024 15:07

nikic reviewed Aug 20, 2024

View reviewed changes

fhahn reviewed Aug 20, 2024

View reviewed changes

madhur13490 force-pushed the madhur13490/correct-lv-info branch from 6777222 to 8bb9dc7 Compare August 21, 2024 11:24

artagnon reviewed Aug 22, 2024

View reviewed changes

madhur13490 force-pushed the madhur13490/correct-lv-info branch from 8bb9dc7 to 01a81a0 Compare August 27, 2024 11:51

madhur13490 requested review from nikic and fhahn August 27, 2024 11:55

fhahn reviewed Aug 27, 2024

View reviewed changes

madhur13490 force-pushed the madhur13490/correct-lv-info branch from 01a81a0 to b99b73e Compare August 27, 2024 16:51

fhahn reviewed Sep 3, 2024

View reviewed changes

madhur13490 force-pushed the madhur13490/correct-lv-info branch from b99b73e to 800f54b Compare September 4, 2024 06:08

fhahn approved these changes Sep 4, 2024

View reviewed changes

madhur13490 added 5 commits September 4, 2024 16:28

[LV] Fix emission of debug message in legality check

bd3064c

Successful vectorization message is emitted even after "Result" is false. "Result" = false indicates failure of one of the legality check and thus successful message should not be printed.

Add test

5eed9bc

Address review comments

b8c7546

Address review comments

7804131

Don't change place of the message

2881500

madhur13490 force-pushed the madhur13490/correct-lv-info branch from 800f54b to 2881500 Compare September 4, 2024 10:58

madhur13490 merged commit cd46829 into llvm:main Sep 4, 2024
5 of 7 checks passed

	; This test asserts that we don't emit both
	; This test checks that we don't emit both successful and unsuccessful

	%indvars.iv = phi i64 [ 1, %entry ], [ %indvars.iv.next, %for.body ]
	%iv = phi i64 [ 1, %entry ], [ %indvars.iv.next, %for.body ]

	%indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
	%iv.next = add nuw nsw i64 %indvars.iv, 1

[LV] Fix emission of debug message in legality check #101924

[LV] Fix emission of debug message in legality check #101924

Uh oh!

Conversation

madhur13490 commented Aug 5, 2024

Uh oh!

llvmbot commented Aug 5, 2024

Uh oh!

github-actions bot commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artagnon commented Aug 7, 2024

Uh oh!

madhur13490 commented Aug 10, 2024

Uh oh!

madhur13490 commented Aug 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

github-actions bot commented Aug 5, 2024 •

edited

Loading