This is an archive of the discontinued LLVM Phabricator instance.

Differential D48968

[DebugInfo][LoopVectorize] Preserve DL in induction PHI and Add
ClosedPublic

Authored by gramanas on Jul 5 2018, 6:41 AM.

Details

Reviewers
vsk
fhahn
hsaito
Commits
rG612bf7cac568: [DebugInfo][LoopVectorize] Preserve DL in induction PHI and Add
rL336667: [DebugInfo][LoopVectorize] Preserve DL in induction PHI and Add
Summary

Continuation of D48769.

In this case the induction phi and add instructions created
in the vector part of the loop were missing DL.

The i8-induction.ll also fails debugify even after this patch:

opt -S -enable-debugify -loop-vectorize i8-induction.ll -disable-output
ERROR: Instruction with empty DebugLoc in function f --  %index = phi i32 [ 0, %vector.ph ], [ %index.next, %vector.body ]
ERROR: Instruction with empty DebugLoc in function f --  %vec.phi = phi <4 x i8> [ <i8 0, i8 1, i8 1, i8 1>, %vector.ph ], [ %5, %vector.body ]
ERROR: Instruction with empty DebugLoc in function f --  %index.next = add i32 %index, 4
ERROR: Instruction with empty DebugLoc in function f --  %9 = icmp eq i32 %index.next, 16
ERROR: Instruction with empty DebugLoc in function f --  br i1 %9, label %middle.block, label %vector.body, !llvm.loop !32
ERROR: Instruction with empty DebugLoc in function f --  %cmp.n = icmp eq i32 16, 16
ERROR: Instruction with empty DebugLoc in function f --  %bc.merge.rdx = phi i8 [ 0, %scalar.ph ], [ %10, %middle.block ]
ERROR: Instruction with empty DebugLoc in function f --  %mul.lcssa = phi i8 [ %mul, %for.body ], [ %10, %middle.block ]
CheckModuleDebugify: FAIL

but those are from different parts of the code so they will be dealt with in
another patch.

Diff Detail

Repository
rL LLVM

Event Timeline

gramanas created this revision.Jul 5 2018, 6:41 AM
Herald added subscribers: llvm-commits, JDevlieghere. · View Herald TranscriptJul 5 2018, 6:41 AM
Harbormaster completed remote builds in B20057: Diff 154222.Jul 5 2018, 6:42 AM
gramanas updated this revision to Diff 154225.Jul 5 2018, 6:45 AM

typo in test

vsk added reviewers: fhahn, hsaito.Jul 5 2018, 11:28 AM
vsk added inline comments.
lib/Transforms/Vectorize/LoopVectorize.cpp
1824 ↗(On Diff #154225)

Why is the debug location of the entry value the correct location to use for an induction variable phi? I'd expect a merged location to be applied to the phi, based on its incoming values.

test/Transforms/LoopVectorize/i8-induction.ll
15 ↗(On Diff #154225)

A reader might not expect to see a DILocation here. Please move this to the end of the file. That's where the metadata would appear in an IR dump.

15 ↗(On Diff #154225)

Please add a comment explaining why the number '5' is here.

gramanas updated this revision to Diff 154399.Jul 6 2018, 6:48 AM
gramanas marked an inline comment as done.

Make test more readable

Harbormaster completed remote builds in B20102: Diff 154399.Jul 6 2018, 6:48 AM
gramanas marked an inline comment as done.Jul 6 2018, 6:48 AM
gramanas added inline comments.
lib/Transforms/Vectorize/LoopVectorize.cpp
1824 ↗(On Diff #154225)

I figured that since this Phi is basically the for loop we are optimizing (in the test case below)
%vec.ind = phi <4 x i8> [ undef, %vector.ph ], [ %vec.ind.next, %vector.body ]
it should get the same DL as the add instruction later.

Also the entry val as mentioned in InnerLoopVectorizer::widenIntOrFpInduction (that calls the function I edited) is
// The value from the original loop to which we are mapping the new induction variable.
that I believe holds the correct DL.

OTOH, I could merge this DL with the vector preHeader BB's terminating instruction that results in this phi. Would this be a better approach?
Basically I could add setDebugLoc(DILocation::getMergedLocation(EntryVal->getDL(), Br->getDL()) after the Phi's incoming values are added, at the end of the function.

test/Transforms/LoopVectorize/i8-induction.ll
15 ↗(On Diff #154225)

I wanted to make sure that it's not an artificial DL. I made it explicit that it's not line: 0 now.

vsk added inline comments.Jul 6 2018, 12:30 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
1824 ↗(On Diff #154225)

Thanks for the explanation. Using EntryVal's location makes sense to me now.

test/Transforms/LoopVectorize/i8-induction.ll
15 ↗(On Diff #154225)

I thought it was better to use line: 5, because it makes it clear that the location comes from "%c.015 = phi i8". A comment would help explain that to readers.

gramanas updated this revision to Diff 154590.Jul 9 2018, 6:37 AM

Use exact line in test

gramanas marked an inline comment as done.Jul 9 2018, 6:37 AM
vsk accepted this revision.Jul 9 2018, 12:03 PM

Thanks, LGTM.

This revision is now accepted and ready to land.Jul 9 2018, 12:03 PM
Closed by commit rL336667: [DebugInfo][LoopVectorize] Preserve DL in induction PHI and Add (authored by gramanas). · Explain WhyJul 10 2018, 6:34 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
trunk/
lib/
Transforms/
Vectorize/
2 lines
test/
Transforms/
LoopVectorize/
10 lines

Diff 154784

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

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 1,815 Lines • ▼ Show 20 Lines Value *SplatVF = isa<Constant>(Mul)
? ConstantVector::getSplat(VF, cast<Constant>(Mul)) ? ConstantVector::getSplat(VF, cast<Constant>(Mul))
: Builder.CreateVectorSplat(VF, Mul); : Builder.CreateVectorSplat(VF, Mul);
Builder.restoreIP(CurrIP); Builder.restoreIP(CurrIP);
// We may need to add the step a number of times, depending on the unroll // We may need to add the step a number of times, depending on the unroll
// factor. The last of those goes into the PHI. // factor. The last of those goes into the PHI.
PHINode *VecInd = PHINode::Create(SteppedStart->getType(), 2, "vec.ind", PHINode *VecInd = PHINode::Create(SteppedStart->getType(), 2, "vec.ind",
&*LoopVectorBody->getFirstInsertionPt()); &*LoopVectorBody->getFirstInsertionPt());
VecInd->setDebugLoc(EntryVal->getDebugLoc());
Instruction *LastInduction = VecInd; Instruction *LastInduction = VecInd;
for (unsigned Part = 0; Part < UF; ++Part) { for (unsigned Part = 0; Part < UF; ++Part) {
VectorLoopValueMap.setVectorValue(EntryVal, Part, LastInduction); VectorLoopValueMap.setVectorValue(EntryVal, Part, LastInduction);
if (isa<TruncInst>(EntryVal)) if (isa<TruncInst>(EntryVal))
addMetadata(LastInduction, EntryVal); addMetadata(LastInduction, EntryVal);
recordVectorLoopValueForInductionCast(II, EntryVal, LastInduction, Part); recordVectorLoopValueForInductionCast(II, EntryVal, LastInduction, Part);
LastInduction = cast<Instruction>(addFastMathFlag( LastInduction = cast<Instruction>(addFastMathFlag(
Builder.CreateBinOp(AddOp, LastInduction, SplatVF, "step.add"))); Builder.CreateBinOp(AddOp, LastInduction, SplatVF, "step.add")));
LastInduction->setDebugLoc(EntryVal->getDebugLoc());
} }
// Move the last step to the end of the latch block. This ensures consistent // Move the last step to the end of the latch block. This ensures consistent
// placement of all induction updates. // placement of all induction updates.
auto *LoopVectorLatch = LI->getLoopFor(LoopVectorBody)->getLoopLatch(); auto *LoopVectorLatch = LI->getLoopFor(LoopVectorBody)->getLoopLatch();
auto *Br = cast<BranchInst>(LoopVectorLatch->getTerminator()); auto *Br = cast<BranchInst>(LoopVectorLatch->getTerminator());
auto *ICmp = cast<Instruction>(Br->getCondition()); auto *ICmp = cast<Instruction>(Br->getCondition());
LastInduction->moveBefore(ICmp); LastInduction->moveBefore(ICmp);
▲ Show 20 Lines • Show All 5,823 Lines • Show Last 20 Lines

llvm/trunk/test/Transforms/LoopVectorize/i8-induction.ll

; RUN: opt < %s -loop-vectorize -force-vector-interleave=1 -force-vector-width=4 -dce -instcombine -S ; RUN: opt < %s -loop-vectorize -force-vector-interleave=1 -force-vector-width=4 -dce -instcombine -S
; RUN: opt < %s -debugify -loop-vectorize -S | FileCheck %s --check-prefix=DEBUGLOC
target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128" target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
@a = common global i8 0, align 1 @a = common global i8 0, align 1
@b = common global i8 0, align 1 @b = common global i8 0, align 1
define void @f() nounwind uwtable ssp { define void @f() nounwind uwtable ssp {
; Check that the induction phis and adds have debug location.
;
; DEBUGLOC-LABEL: vector.body:
; DEBUGLOC: %vec.ind = phi {{.*}}, !dbg ![[DbgLoc:[0-9]+]]
; DEBUGLOC: %vec.ind.next = add {{.*}}, !dbg ![[DbgLoc]]
scalar.ph: scalar.ph:
store i8 0, i8* inttoptr (i64 1 to i8*), align 1 store i8 0, i8* inttoptr (i64 1 to i8*), align 1
%0 = load i8, i8* @a, align 1 %0 = load i8, i8* @a, align 1
br label %for.body br label %for.body
for.body: for.body:
%mul16 = phi i8 [ 0, %scalar.ph ], [ %mul, %for.body ] ; <------- i8 induction var. %mul16 = phi i8 [ 0, %scalar.ph ], [ %mul, %for.body ] ; <------- i8 induction var.
%c.015 = phi i8 [ undef, %scalar.ph ], [ %conv8, %for.body ] %c.015 = phi i8 [ undef, %scalar.ph ], [ %conv8, %for.body ]
%conv2 = sext i8 %c.015 to i32 %conv2 = sext i8 %c.015 to i32
%tobool = icmp ne i8 %c.015, 0 %tobool = icmp ne i8 %c.015, 0
%.sink = select i1 %tobool, i8 %c.015, i8 %0 %.sink = select i1 %tobool, i8 %c.015, i8 %0
%mul = mul i8 %mul16, %.sink %mul = mul i8 %mul16, %.sink
%add = add nsw i32 %conv2, 1 %add = add nsw i32 %conv2, 1
%conv8 = trunc i32 %add to i8 %conv8 = trunc i32 %add to i8
%sext = shl i32 %add, 24 %sext = shl i32 %add, 24
%phitmp14 = icmp slt i32 %sext, 268435456 %phitmp14 = icmp slt i32 %sext, 268435456
br i1 %phitmp14, label %for.body, label %for.end br i1 %phitmp14, label %for.body, label %for.end
for.end: ; preds = %for.body for.end: ; preds = %for.body
store i8 %mul, i8* @b, align 1 store i8 %mul, i8* @b, align 1
ret void ret void
} }
; Check that the location of the new phi comes from %c.015 = phi i8
; DEBUGLOC: ![[DbgLoc]] = !DILocation(line: 5