Page MenuHomePhabricator

[Unroll][DebugInfo] Propagate loop body's debug location to epilog preheader
ClosedPublic

Authored by zzheng on Dec 20 2017, 2:28 PM.

Details

Summary

NewExit and epilog PreHeader should has the same debug loc as the original loop
body, instead of original loop exit.

Diff Detail

Repository
rL LLVM

Event Timeline

zzheng created this revision.Dec 20 2017, 2:28 PM

I'll defer to Dehao. Can you see whether this messes with SBPGO?

probinson added subscribers: andreadb, probinson.
probinson added inline comments.
test/Transforms/LoopUnroll/runtime-loop1.ll
16

The test is now defining BODY_LOC three times in the EPILOG run. Once should be enough. (The unmodified test incorrectly defines it twice, and you've added a third.)

zzheng updated this revision to Diff 127808.Dec 20 2017, 5:34 PM

Removed redefinition of BODY_LOC in test case runtime-loop1.ll

zzheng marked an inline comment as done.Dec 20 2017, 5:34 PM
evstupac added inline comments.Dec 21 2017, 12:54 PM
lib/Transforms/Utils/LoopUnrollRuntime.cpp
651–652

Am I right that this will set more DebugLoc for epilog case than for prolog? If so, why?
If "TI" is supposed to be used only once I wouldn't create it at all:

NewExit->getTerminator()->setDebugLoc(
    Header->getTerminator()->getDebugLoc());

If not - reuse it in the line 654 and give wider name (like NewExitTerminator).
EpilogPreHeader = SplitBlock(NewExit, NewExitTerminator, DT, LI);

test/Transforms/LoopUnroll/runtime-epilog-debuginfo.ll
3

Please add a comment on what test is checking.

zzheng added inline comments.Dec 21 2017, 5:34 PM
lib/Transforms/Utils/LoopUnrollRuntime.cpp
651–652

NewExit gets its DebugLoc from LatchExit. This DebugLoc is not part of the original loop, it's after the original loop (say loc A).

The conditional branch deciding if epilog will be added to NewExit, which should be part of the original loop in lexical sense.

If a later pass search for debuginfo in RPOT order, it will first find loc A at the conditional branch rather than the merge point of unrolled and epilog loop.

On the other hand, PrologPreHeader gets its debug info from original PreHeader.

zzheng updated this revision to Diff 127973.Dec 21 2017, 5:39 PM
zzheng retitled this revision from [Unroll][DebugInfo] Propagate loop body's debug info to epilog preheader to [Unroll][DebugInfo] Propagate loop body's debug location to epilog preheader.

Addressed Evgeny's comments.

zzheng updated this revision to Diff 127975.Dec 21 2017, 5:41 PM
zzheng marked an inline comment as done.
evstupac added inline comments.Dec 21 2017, 5:47 PM
lib/Transforms/Utils/LoopUnrollRuntime.cpp
651–652

Thanks for detailed explanation. Now it is clear for me. I'd add this as a comment:
NewExit gets its DebugLoc from LatchExit, which do not belongs to the Loop.
Fix this by setting Loop DebugLoc to NewExit.

The change makes sense to me. It should not affect SamplePGO as terminator instructions is not used in profile annotation.

zzheng updated this revision to Diff 128093.Dec 23 2017, 5:27 PM

Added comments as Evgeny suggested.

zzheng marked 2 inline comments as done.Dec 23 2017, 5:27 PM

Ping... I'd like to commit this soon if there's no further suggestion for revision.

evstupac accepted this revision.Dec 26 2017, 12:38 PM
This revision is now accepted and ready to land.Dec 26 2017, 12:38 PM
zzheng closed this revision.Dec 26 2017, 3:34 PM

Committed as r321465.