Page MenuHomePhabricator

[llvm] API for encoding/decoding DWARF discriminators.
ClosedPublic

Authored by mtrofin on Dec 13 2018, 3:38 PM.

Details

Summary

Added a pair of APIs for encoding/decoding the 3 components of a DWARF discriminator described in http://lists.llvm.org/pipermail/llvm-dev/2016-October/106532.html: the base discriminator, the duplication factor (useful in profile-guided optimization) and the copy index (used to identify copies of code in cases like loop unrolling)

The encoding packs 3 unsigned values in 32 bits. This CL addresses 2 issues:

  • communicates overflow back to the user
  • supports encoding all 3 components together. Current APIs assume a sequencing of events. For example, creating a new discriminator based on an existing one by changing the base discriminator was not supported.

Diff Detail

Repository
rL LLVM

Event Timeline

mtrofin created this revision.Dec 13 2018, 3:38 PM
mtrofin updated this revision to Diff 178156.Dec 13 2018, 3:45 PM
  • Remove unrelated changes (clang-format)
aprantl added inline comments.Dec 14 2018, 8:18 AM
include/llvm/IR/DebugInfoMetadata.h
1607 ↗(On Diff #178156)

Could you please move all the implementations to DebugInfoMetadata.cpp ?

mtrofin updated this revision to Diff 178238.Dec 14 2018, 8:49 AM
  • Moved implementation of new APIs to DebugInfoMetadata.cpp.
mtrofin marked 2 inline comments as done.Dec 14 2018, 8:51 AM
mtrofin added inline comments.
include/llvm/IR/DebugInfoMetadata.h
1607 ↗(On Diff #178156)

Thanks - I assume you were referring to the new encode/decode pair.

aprantl added inline comments.Dec 14 2018, 10:08 AM
include/llvm/IR/DebugInfoMetadata.h
35 ↗(On Diff #178238)

Is this still needed?

mtrofin updated this revision to Diff 178294.Dec 14 2018, 2:33 PM
mtrofin marked 2 inline comments as done.
  • Adjust includes

Gentle reminder - thanks!

wmi added a comment.Dec 17 2018, 11:22 AM

LGTM from my limited knowledge on discriminator. I added dblaikie to help on the review.

lib/IR/DebugInfoMetadata.cpp
140–141 ↗(On Diff #178294)

Maybe make the comment more explicitly that encode may fail due to overflow of certain component. Currently the overflow failure is detected by checking the equivalence of all the components before encoding and after decoding.

mtrofin updated this revision to Diff 178503.Dec 17 2018, 11:29 AM
  • Improved comments.

Might be worth having a summary of the goals here (rather than only by reference to the design discussion thread) for review and later as commit history, etc.

include/llvm/IR/DebugInfoMetadata.h
1605–1606 ↗(On Diff #178503)

Would Expected<unsigned> or Optional<unsigned> be more suitable here rather than using a bool as an out parameter to report errors?

mtrofin edited the summary of this revision. (Show Details)Dec 19 2018, 3:40 PM
mtrofin updated this revision to Diff 179015.Dec 19 2018, 9:11 PM
mtrofin marked an inline comment as done.
  • Returning Optional<unsigned>
mtrofin marked 2 inline comments as done.Dec 19 2018, 9:12 PM
mtrofin added inline comments.
include/llvm/IR/DebugInfoMetadata.h
1605–1606 ↗(On Diff #178503)

Done - also adjusted current APIs for consistency.

mtrofin marked an inline comment as done.Dec 21 2018, 8:15 AM

ptal - thanks!

dblaikie accepted this revision.Dec 21 2018, 2:05 PM

Not sure I'm following this all in detail & at least for me (someone not super familiar with the details here) it might've been a bit easier to review in smaller pieces (breaking up the refactoring to generalize the components of the discriminator into smaller bits, etc) but all good. Thanks!

include/llvm/IR/DebugInfoMetadata.h
2057–2058 ↗(On Diff #179015)

You could roll the variable declaration into the condition:

if (Optional<unsigned> D = ...)
2059 ↗(On Diff #179015)

If you like, you can use *D instead of D.getValue()

lib/IR/DebugInfoMetadata.cpp
152–155 ↗(On Diff #179015)

Perhaps these should call getBaseDiscriminatorFromDiscriminator, getDuplicationFactorFromDiscriminator, getCopyIdentifierFromDiscriminator?

(though it looks like getDuplicationFactorFromdiscriminator has some extra logic that isn't in the initialization for DF here in decodeDiscriminator - is that a bug that these two cases are handled differently?)

lib/Target/X86/X86DiscriminateMemOps.cpp
138 ↗(On Diff #179015)

I'd probably drop the top level 'const' here (since it's a value rather than a reference - LLVM doesn't usually use top level const like this)

This revision is now accepted and ready to land.Dec 21 2018, 2:05 PM
mtrofin updated this revision to Diff 179356.Dec 21 2018, 2:25 PM
mtrofin marked 6 inline comments as done.
  • Remove unrelated changes (clang-format)
  • Moved implementation of new APIs to DebugInfoMetadata.cpp.
  • Adjust includes
  • Improved comments.
  • Returning Optional<unsigned>
  • Feedback
mtrofin added inline comments.Dec 21 2018, 2:26 PM
lib/IR/DebugInfoMetadata.cpp
152–155 ↗(On Diff #179015)

Yes, the current APIs are a bit special-casing. I decided to leave them as-is, and just offer decodeDiscriminator as access to the raw data.

lib/Target/X86/X86DiscriminateMemOps.cpp
138 ↗(On Diff #179015)

I wanted to make sure no mutation happens through the iterator. I'll remove it for consistency though.

This revision was automatically updated to reflect the committed changes.