This is an archive of the discontinued LLVM Phabricator instance.

Dump Accumulator
Needs RevisionPublic

Authored by kazu on Jul 23 2020, 4:12 PM.

Details

Summary

This patch adds a utility called DumpAccumulator.

DumpAccumulator allows you to dump arbitrary text messages into a special
section called .llvm_dump. The linker then concatenates these messages into
the identically named section in the final executable.

This utility makes it easy to collect information from optimization
passes of interest in a build environment that caches compilation.

See llvm/include/llvm/Analysis/DumpAccumulator.h for usage.

The original implementation is from Mircea Trofin, and I generalized it.

Diff Detail

Event Timeline

kazu created this revision.Jul 23 2020, 4:12 PM
mtrofin added inline comments.Jul 23 2020, 4:19 PM
llvm/include/llvm/Analysis/DumpAccumulator.h
43

I'd clarify that transferring the data from pre-link thinlto to post-link isn't supported, but one could use this in post-link, correct?

kazu marked an inline comment as done.Jul 27 2020, 1:30 PM
kazu added inline comments.
llvm/include/llvm/Analysis/DumpAccumulator.h
43

We don't add DumpAccumulator to the post-link ThinLTO pipeline yet.

Note that we add DumpAccumulator in buildPerModuleDefaultPipeline, but post-link ThinLTO doesn't call the function.

mtrofin added inline comments.Jul 27 2020, 2:27 PM
llvm/include/llvm/Analysis/DumpAccumulator.h
43

Yes - my point is that one could enable it in post-link and 'it'd just work'; the thing that doesn't work is making pre-link data jump over to post-link, which would require more plumbing.

mtrofin accepted this revision.Jul 27 2020, 2:27 PM

lgtm

This revision is now accepted and ready to land.Jul 27 2020, 2:27 PM

Could the llvm-readobj bit be spun off into a separate review, please. As things stand it doesn't have any of its own testing, which should be added at the same time as adding support in the tool itself. The new option is also lacking any documentation in the llvm-readobj and llvm-readelf docs.

kazu added a comment.Jul 28 2020, 1:21 PM

Could the llvm-readobj bit be spun off into a separate review, please. As things stand it doesn't have any of its own testing, which should be added at the same time as adding support in the tool itself. The new option is also lacking any documentation in the llvm-readobj and llvm-readelf docs.

Sure. I'll split the compiler and llvm-readobj bits and check in the former first. I'll come up with documentation for the llvm-readobj bit. Thanks!

kazu updated this revision to Diff 281344.Jul 28 2020, 1:22 PM

Spun off the llvm-readobj bit.

jhenderson requested changes to this revision.Jul 29 2020, 4:49 AM

I'm assuming you need llvm-readobj to test your changes? In which case, the llvm-readobj change needs to come first, and then you can write tests using it to test this change (at the moment, the testing for this seems insufficient to me). Do you have a patch yet for the llvm-readobj side?

Could you add some documentation somewhere, either in code comments or somewhere else, explaining the output format you actually want this to be? From my casual reading (I'm not the most knowledgeable of people in this area), it looks like you're creating an ELF SHT_NOTE section, but don't use the SHT_NOTE ELF format (see https://www.sco.com/developers/gabi/latest/ch5.pheader.html#note_section for details). I would actually expect this to emit a section with a new section type, so that the consumer (llvm-readobj) can more easily identify it. Alternatively, it could just be a basic SHT_NOTE section, with a specific note type. Under no circumstances should consumers need to check the section header name to identify the section (string comparisons are expensive, and should be avoided where possible - the general intent for ELF is for different kinds of sections to be distinguished by their types and flags).

llvm/include/llvm/Analysis/DumpAccumulator.h
57

Spurious include?

This revision now requires changes to proceed.Jul 29 2020, 4:49 AM

By the way, has the design of this been discussed on llvm-dev? It seems like something should have an RFC if it hasn't already.