Page MenuHomePhabricator

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

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

Details

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
gramanas updated this revision to Diff 154225.Jul 5 2018, 6:45 AM

typo in test

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

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
This revision was automatically updated to reflect the committed changes.