Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 24, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
59831d6
[LV] Align debug location of the widen-phi to the orignal phi.
ElvisWang123 Dec 18, 2024
4982637
!fixup. Test the widen-phi has same debugLoc with scalar counterpart.
ElvisWang123 Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions llvm/lib/Transforms/Vectorize/VPlan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2310,9 +2310,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) {
Comment on lines +2313 to +2316
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()) {

Copy link
Contributor Author

@ElvisWang123 ElvisWang123 Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Mel-Chen Mel-Chen Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (Start)
addOperand(Start);
}
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3547,6 +3547,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");
Expand Down
9 changes: 5 additions & 4 deletions llvm/test/Transforms/LoopVectorize/dbg-outer-loop-vect.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]] ], !dbg [[DBG34:![0-9]+]]
; 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]]
Expand All @@ -42,7 +42,7 @@ define void @foo(ptr %h) !dbg !4 {
; CHECK-NEXT: #dbg_value(i64 [[I_023]], [[META11]], !DIExpression(), [[META20]])
; CHECK-NEXT: br label [[FOR_COND5_PREHEADER:%.*]], !dbg [[DBG26]]
; CHECK: for.cond5.preheader:
; CHECK-NEXT: [[L_022:%.*]] = phi i64 [ 0, [[FOR_COND1_PREHEADER]] ], [ [[INC10:%.*]], [[FOR_COND5_PREHEADER]] ]
; CHECK-NEXT: [[L_022:%.*]] = phi i64 [ 0, [[FOR_COND1_PREHEADER]] ], [ [[INC10:%.*]], [[FOR_COND5_PREHEADER]] ], !dbg [[DBG34]]
; CHECK-NEXT: [[TMP10:%.*]] = getelementptr i32, ptr [[H]], i64 [[L_022]]
; CHECK-NEXT: store i32 0, ptr [[TMP10]], align 4, !dbg [[DBG22]]
; CHECK-NEXT: [[ARRAYIDX_1:%.*]] = getelementptr i32, ptr [[TMP10]], i64 1, !dbg [[DBG31:![0-9]+]]
Expand Down Expand Up @@ -72,7 +72,7 @@ for.cond1.preheader: ; preds = %entry, %for.cond.cl
br label %for.cond5.preheader, !dbg !22

for.cond5.preheader: ; preds = %for.cond1.preheader, %for.cond5.preheader
%l.022 = phi i64 [ 0, %for.cond1.preheader ], [ %inc10, %for.cond5.preheader ]
%l.022 = phi i64 [ 0, %for.cond1.preheader ], [ %inc10, %for.cond5.preheader ], !dbg !34
%0 = getelementptr i32, ptr %h, i64 %l.022
store i32 0, ptr %0, align 4, !dbg !24
%arrayidx.1 = getelementptr i32, ptr %0, i64 1, !dbg !26
Expand Down Expand Up @@ -134,6 +134,7 @@ declare void @llvm.dbg.value(metadata, metadata, metadata)
!31 = distinct !{!31, !21, !32, !33}
!32 = !DILocation(line: 13, column: 13, scope: !12)
!33 = !{!"llvm.loop.vectorize.enable", i1 true}
!34 = !DILocation(line: 10, column: 5, scope: !12)
;.
; CHECK: [[META0:![0-9]+]] = distinct !DICompileUnit(language: DW_LANG_C11, file: [[META1:![0-9]+]], producer: "clang", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
; CHECK: [[META1]] = !DIFile(filename: "outer-loop-vect.c", directory: {{.*}})
Expand Down
Loading