[LV] Align debug location of the widen-phi to the original phi. #120338

ElvisWang123 · 2024-12-18T00:06:18Z

This patch align the debug location of the widen-phi to the debug location of original phi.

Split from: #120054

llvmbot · 2024-12-18T00:06:54Z

@llvm/pr-subscribers-vectorizers

@llvm/pr-subscribers-llvm-transforms

Author: Elvis Wang (ElvisWang123)

Changes

This patch align the debug location of the widen-phi to the debug location of original phi.

Split from: #120054

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

4 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/VPlan.h (+4-3)
(modified) llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp (+1-1)
(modified) llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp (+1)
(modified) llvm/test/Transforms/LoopVectorize/dbg-outer-loop-vect.ll (+2-2)

diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 12208a7968338b..2f1f3f5745c8ae 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -2311,9 +2311,10 @@ class VPWidenPHIRecipe : public VPSingleDefRecipe {
   SmallVector<VPBasicBlock *, 2> IncomingBlocks;
 
 public:
-  /// Create a new VPWidenPHIRecipe for \p Phi with start value \p Start.
-  VPWidenPHIRecipe(PHINode *Phi, VPValue *Start = nullptr)
-      : VPSingleDefRecipe(VPDef::VPWidenPHISC, ArrayRef<VPValue *>(), Phi) {
+  /// Create a new VPWidenPHIRecipe for \p Phi with start value \p Start and
+  /// debug location \p DL.
+  VPWidenPHIRecipe(PHINode *Phi, VPValue *Start = nullptr, DebugLoc DL = {})
+      : VPSingleDefRecipe(VPDef::VPWidenPHISC, ArrayRef<VPValue *>(), Phi, DL) {
     if (Start)
       addOperand(Start);
   }
diff --git a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
index 6e633739fcc3dd..140cea3c700d87 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
@@ -308,7 +308,7 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB,
       // Phi node's operands may have not been visited at this point. We create
       // an empty VPInstruction that we will fix once the whole plain CFG has
       // been built.
-      NewVPV = new VPWidenPHIRecipe(Phi);
+      NewVPV = new VPWidenPHIRecipe(Phi, nullptr, Phi->getDebugLoc());
       VPBB->appendRecipe(cast<VPWidenPHIRecipe>(NewVPV));
       PhisToFix.push_back(Phi);
     } else {
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index ab95b647f211b7..b695d1419ca4e7 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -3456,6 +3456,7 @@ void VPWidenPHIRecipe::execute(VPTransformState &State) {
   assert(EnableVPlanNativePath &&
          "Non-native vplans are not expected to have VPWidenPHIRecipes.");
 
+  State.setDebugLocFrom(getDebugLoc());
   Value *Op0 = State.get(getOperand(0));
   Type *VecTy = Op0->getType();
   Value *VecPhi = State.Builder.CreatePHI(VecTy, 2, "vec.phi");
diff --git a/llvm/test/Transforms/LoopVectorize/dbg-outer-loop-vect.ll b/llvm/test/Transforms/LoopVectorize/dbg-outer-loop-vect.ll
index 66aceab9fb27c8..44afa34100c299 100644
--- a/llvm/test/Transforms/LoopVectorize/dbg-outer-loop-vect.ll
+++ b/llvm/test/Transforms/LoopVectorize/dbg-outer-loop-vect.ll
@@ -15,8 +15,8 @@ define void @foo(ptr %h) !dbg !4 {
 ; CHECK-NEXT:    [[INDEX:%.*]] = phi i64 [ 0, [[VECTOR_PH]] ], [ [[INDEX_NEXT:%.*]], [[FOR_COND_CLEANUP32:%.*]] ]
 ; CHECK-NEXT:    br label [[FOR_COND5_PREHEADER1:%.*]], !dbg [[DBG21]]
 ; CHECK:       for.cond5.preheader1:
-; CHECK-NEXT:    [[VEC_PHI:%.*]] = phi <4 x i64> [ zeroinitializer, [[VECTOR_BODY]] ], [ [[TMP4:%.*]], [[FOR_COND5_PREHEADER1]] ], !dbg [[DBG21]]
-; CHECK-NEXT:    [[TMP0:%.*]] = getelementptr i32, ptr [[H]], <4 x i64> [[VEC_PHI]], !dbg [[DBG21]]
+; CHECK-NEXT:    [[VEC_PHI:%.*]] = phi <4 x i64> [ zeroinitializer, [[VECTOR_BODY]] ], [ [[TMP4:%.*]], [[FOR_COND5_PREHEADER1]] ]
+; CHECK-NEXT:    [[TMP0:%.*]] = getelementptr i32, ptr [[H]], <4 x i64> [[VEC_PHI]]
 ; CHECK-NEXT:    call void @llvm.masked.scatter.v4i32.v4p0(<4 x i32> zeroinitializer, <4 x ptr> [[TMP0]], i32 4, <4 x i1> splat (i1 true)), !dbg [[DBG22:![0-9]+]]
 ; CHECK-NEXT:    [[TMP1:%.*]] = getelementptr i32, <4 x ptr> [[TMP0]], i64 1, !dbg [[DBG22]]
 ; CHECK-NEXT:    call void @llvm.masked.scatter.v4i32.v4p0(<4 x i32> splat (i32 1), <4 x ptr> [[TMP1]], i32 4, <4 x i1> splat (i1 true)), !dbg [[DBG22]]

lukel97

LGTM but best to wait for someone else to approve

Mel-Chen · 2024-12-19T07:05:29Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  /// Create a new VPWidenPHIRecipe for \p Phi with start value \p Start and
+  /// debug location \p DL.
+  VPWidenPHIRecipe(PHINode *Phi, VPValue *Start = nullptr, DebugLoc DL = {})
+      : VPSingleDefRecipe(VPDef::VPWidenPHISC, ArrayRef<VPValue *>(), Phi, DL) {


Why not pass debug info by this way? Could Phi be nullptr, or other reasons?

Suggested change

/// Create a new VPWidenPHIRecipe for \p Phi with start value \p Start and

/// debug location \p DL.

VPWidenPHIRecipe(PHINode *Phi, VPValue *Start = nullptr, DebugLoc DL = {})

: VPSingleDefRecipe(VPDef::VPWidenPHISC, ArrayRef<VPValue *>(), Phi, DL) {

/// Create a new VPWidenPHIRecipe for \p Phi with start value \p Start.

VPWidenPHIRecipe(PHINode *Phi, VPValue *Start = nullptr)

: VPSingleDefRecipe(VPDef::VPWidenPHISC, ArrayRef<VPValue *>(), Phi, Phi->getDebugLoc()) {

Good point! It is fine to use phi->getDebugLoc() since we don't construct VPWidenPHIRecipe with nullptr currently. But do we guarantee the phi must not be a nullptr?

I implemented #120054 similar to your suggested method initially but found that we construct VPReductionRecipe in unit-test with nullptr. And I opened #120053 to prevent nullptr when create VPReductionRecipe in unit-test.
@fhahn suggested that we could pass the debug information independent of underlying instruction (#120054 (comment)) to prevent unit-test changes.

I see, considering that Phi could be nullptr is reasonable.
But I have another question: Do we allow phi->getDebugLoc() and DL to be different? If not, should we add an assert in the constructor? If we do allow it, should we use phi->getDebugLoc() (if phi is not nullptr) or DL?
cc: @fhahn

IMO it is better to pass the debug loc separately; the phi instruction is technically optionally

lukel97 · 2025-01-06T10:16:03Z

Gentle ping by proxy. This looks similar to what was done in f4230b4

fhahn · 2025-01-06T19:10:14Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  /// Create a new VPWidenPHIRecipe for \p Phi with start value \p Start and
+  /// debug location \p DL.
+  VPWidenPHIRecipe(PHINode *Phi, VPValue *Start = nullptr, DebugLoc DL = {})
+      : VPSingleDefRecipe(VPDef::VPWidenPHISC, ArrayRef<VPValue *>(), Phi, DL) {


IMO it is better to pass the debug loc separately; the phi instruction is technically optionally

fhahn · 2025-01-06T19:11:34Z

llvm/test/Transforms/LoopVectorize/dbg-outer-loop-vect.ll

@@ -15,8 +15,8 @@ define void @foo(ptr %h) !dbg !4 {
 ; CHECK-NEXT:    [[INDEX:%.*]] = phi i64 [ 0, [[VECTOR_PH]] ], [ [[INDEX_NEXT:%.*]], [[FOR_COND_CLEANUP32:%.*]] ]
 ; CHECK-NEXT:    br label [[FOR_COND5_PREHEADER1:%.*]], !dbg [[DBG21]]
 ; CHECK:       for.cond5.preheader1:
-; CHECK-NEXT:    [[VEC_PHI:%.*]] = phi <4 x i64> [ zeroinitializer, [[VECTOR_BODY]] ], [ [[TMP4:%.*]], [[FOR_COND5_PREHEADER1]] ], !dbg [[DBG21]]
-; CHECK-NEXT:    [[TMP0:%.*]] = getelementptr i32, ptr [[H]], <4 x i64> [[VEC_PHI]], !dbg [[DBG21]]
+; CHECK-NEXT:    [[VEC_PHI:%.*]] = phi <4 x i64> [ zeroinitializer, [[VECTOR_BODY]] ], [ [[TMP4:%.*]], [[FOR_COND5_PREHEADER1]] ]


Just noting that this is because the phi doesn't have a debug location in the original source.

Could you make sure we have a wide phi with a debug location to have test coverage for setting a correct non-empty debug location?

I added a virtual debugLoc to the original scalar phi in this test case and added tests to make sure widen-phi has same debugLoc with the scalar counterpart.

lukel97 · 2025-01-24T02:04:03Z

Reverse ping cc @ElvisWang123

ElvisWang123 · 2025-01-24T02:07:07Z

Reverse ping cc @ElvisWang123

Oops, I missed the review. Working on it.

This patch align the debug location of the widen-phi to the debug location of original phi.

fhahn

LGTM, thanks

ElvisWang123 requested a review from fhahn December 18, 2024 00:06

llvmbot added vectorizers llvm:transforms labels Dec 18, 2024

ElvisWang123 mentioned this pull request Dec 18, 2024

[VPlan] Set debug location for VPReduction/VPWidenIntrinsicRecipe. #120054

Merged

ElvisWang123 requested a review from lukel97 December 18, 2024 00:09

lukel97 reviewed Dec 18, 2024

View reviewed changes

Mel-Chen reviewed Dec 19, 2024

View reviewed changes

lukel97 mentioned this pull request Jan 3, 2025

[VPlan] Allow VPWidenPHI in non-native path. NFC #118662

Closed

lukel97 mentioned this pull request Jan 6, 2025

[VPlan] Expand VPWidenIntOrFpInductionRecipe into separate recipes #118638

Open

fhahn reviewed Jan 6, 2025

View reviewed changes

ElvisWang123 added 2 commits January 23, 2025 19:21

[LV] Align debug location of the widen-phi to the orignal phi.

59831d6

This patch align the debug location of the widen-phi to the debug location of original phi.

!fixup. Test the widen-phi has same debugLoc with scalar counterpart.

4982637

ElvisWang123 force-pushed the fix-dbg-loc-VPWidenPHIRecipe branch from 2a1a11f to 4982637 Compare January 24, 2025 05:17

fhahn approved these changes Jan 24, 2025

View reviewed changes

ElvisWang123 merged commit aff1242 into llvm:main Jan 24, 2025
7 checks passed

ElvisWang123 deleted the fix-dbg-loc-VPWidenPHIRecipe branch January 24, 2025 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[LV] Align debug location of the widen-phi to the original phi. #120338

[LV] Align debug location of the widen-phi to the original phi. #120338

Uh oh!

ElvisWang123 commented Dec 18, 2024

Uh oh!

llvmbot commented Dec 18, 2024 •

edited

Loading

Uh oh!

lukel97 left a comment

Uh oh!

Mel-Chen Dec 19, 2024

Uh oh!

ElvisWang123 Dec 19, 2024 •

edited

Loading

Uh oh!

Mel-Chen Dec 20, 2024 •

edited

Loading

Uh oh!

fhahn Jan 6, 2025

Uh oh!

lukel97 commented Jan 6, 2025

Uh oh!

fhahn Jan 6, 2025

Uh oh!

fhahn Jan 6, 2025

Uh oh!

ElvisWang123 Jan 24, 2025

Uh oh!

lukel97 commented Jan 24, 2025

Uh oh!

ElvisWang123 commented Jan 24, 2025

Uh oh!

fhahn left a comment

Uh oh!

Uh oh!

Uh oh!

[LV] Align debug location of the widen-phi to the original phi. #120338

[LV] Align debug location of the widen-phi to the original phi. #120338

Uh oh!

Conversation

ElvisWang123 commented Dec 18, 2024

Uh oh!

llvmbot commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukel97 left a comment

Choose a reason for hiding this comment

Uh oh!

Mel-Chen Dec 19, 2024

Choose a reason for hiding this comment

Uh oh!

ElvisWang123 Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mel-Chen Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhahn Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

lukel97 commented Jan 6, 2025

Uh oh!

fhahn Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

fhahn Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

ElvisWang123 Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

lukel97 commented Jan 24, 2025

Uh oh!

ElvisWang123 commented Jan 24, 2025

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Dec 18, 2024 •

edited

Loading

ElvisWang123 Dec 19, 2024 •

edited

Loading

Mel-Chen Dec 20, 2024 •

edited

Loading