This is an archive of the discontinued LLVM Phabricator instance.

Support using sample profiles with partial debug info.
ClosedPublic

Authored by dnovillo on Oct 21 2014, 12:10 PM.

Details

Summary

When using a profile, we used to require the use -gmlt so that we could
get access to the line locations. This is used to match line numbers in
the input profile to the line numbers in the function's IR.

But this is actually not necessary. The driver can provide source
location tracking without the emission of debug information. In these
cases, the annotation 'llvm.dbg.cu' is missing from the IR, but the
actual line location annotations are still present.

This patch adds a new way of looking for the start of the current
function. Instead of looking through the compile units in llvm.dbg.cu,
we can walk up the scope for the first instruction in the function with
a debug loc. If that describes the function, we use it. Otherwise, we
keep looking until we find one.

If no such instruction is found, we then give up and produce a warning.
I changed the diagnostic from an error to a warning because it's not
really a codegen problem. The compiler can continue, it's just that the
optimization opportunities won't include profile information.

Diff Detail

Event Timeline

dnovillo updated this revision to Diff 15199.Oct 21 2014, 12:10 PM
dnovillo retitled this revision from to Support using sample profiles with partial debug info..
dnovillo updated this object.
dnovillo edited the test plan for this revision. (Show Details)
dnovillo added reviewers: echristo, dblaikie.
dnovillo added a subscriber: Unknown Object (MLST).
echristo accepted this revision.Oct 21 2014, 12:36 PM
echristo edited edge metadata.

No need for the \p but otherwise it's probably ok. If you could put the actual code in the test that would be nice.

Thanks!

This revision is now accepted and ready to land.Oct 21 2014, 12:36 PM
dblaikie added inline comments.Oct 21 2014, 12:38 PM
lib/Transforms/Scalar/SampleProfile.cpp
672

Please use range-based-for loops.

681

If you find an instruction with DebugLoc, you don't need to keep searching - no matter what its subprogram node is.

This is what I tried to describe with the pseudocode I mentioned on IRC the other day.

for (basic blocks)

for (instructions)
  if (debugloc.isvalid)
    if (getsubprogram(debugloc).describes(F))
      return subprogram.getLineNumber()
    else
      /* break out of all the loops & consider this a failure */

(maybe wrap all this in a function so you can more easily early-exit when you reach that situation)

Here's the invariant that I believe now holds:
If a function has debug info, the scope chain of all instructions in that function will lead to the function and nothing else.

So once you find one instruction with a debug loc, you don't need to examine any others - if it leads to this function, you're done, if it doesn't then this function doesn't have debug info.

test/Transforms/SampleProfile/loc-tracking-only.ll
3 ↗(On Diff #15199)

Worth updating calls.ll itself instead of adding a new test? If you don't depend on the llvm.dbg.cu at all, it doesn't seem worthwhile to have two different tests, one with it and one without it.

dnovillo updated this revision to Diff 15215.Oct 21 2014, 3:05 PM
dnovillo edited edge metadata.
  • Re-factor DISubprogram locator code.
  • Remove unecessary test.
dblaikie added inline comments.Oct 21 2014, 3:24 PM
lib/Transforms/Scalar/SampleProfile.cpp
668–669

Why do we need this code at all? Should we just remove it in favor of the other code?

I'd have thought it'd be faster too.

dnovillo updated this revision to Diff 15219.Oct 21 2014, 3:51 PM
  • Do not try to use llvm.dbg.cu to find the subprogram for F.
dblaikie accepted this revision.Oct 21 2014, 3:55 PM
dblaikie edited edge metadata.

Some optional tidbits, but otherwise fine.

lib/Transforms/Scalar/SampleProfile.cpp
646

usually we do this with an early continue to avoid extra indentation

if (DLoc.isKnown())
  continue;
/* more stuff */

& yeah, no worries about the range-for loops, a single cleanup would be fine

675

Separating the change in text and error->warning (& the subsequent test changes) into a separate commit would be nice.

dnovillo closed this revision.Oct 22 2014, 6:09 AM
dnovillo updated this revision to Diff 15239.

Closed by commit rL220382 (authored by @dnovillo).