[LV] Align debug location of the widen-phi to the original phi. by ElvisWang123 · Pull Request #120338 · llvm/llvm-project

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

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

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

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

Merged

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

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

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

llvmbot commented Dec 18, 2024 •

edited

Loading

ElvisWang123 Dec 19, 2024 •

edited

Loading

Mel-Chen Dec 20, 2024 •

edited

Loading