Page MenuHomePhabricator

[WIP] New AsmPrinterHandler that is able to interleave source code
Needs ReviewPublic

Authored by rogfer01 on Mar 13 2017, 9:23 AM.

Details

Summary

This is the LLVM side of the proposal in http://lists.llvm.org/pipermail/cfe-dev/2017-February/052549.html

Basically introduces a new AsmPrinterHandler that is enabled when AsmSource is not None. AsmSource is not just a boolean because we do not want to clutter the assembler with debug directives if we are just using -fverbose-asm (and no other debug information has been requested).

See the clang side of this in https://reviews.llvm.org/D30898

*TODO*: Add tests

Diff Detail

Event Timeline

rogfer01 created this revision.Mar 13 2017, 9:23 AM
rogfer01 edited the summary of this revision. (Show Details)Mar 13 2017, 9:31 AM
rogfer01 edited the summary of this revision. (Show Details)
fpetrogalli edited edge metadata.Mar 14 2017, 2:08 AM

Hi Roger,

thank you for working on this. I have some minor comments, good job!

A test would also make clear what would be the desired output of this new functionality.

Francesco

include/llvm/MC/MCTargetOptions.h
26–31

Please describe this enumeration.

lib/CodeGen/AsmPrinter/SourceInterleave.cpp
55–64

You could use DebugLoc::print (or factor out part of it if you don't need column and line number).

99

Nitpicking: const std:string?

107

Nitpicking: const int?

136

}// end namespace llvm

efriedma added inline comments.
lib/CodeGen/AsmPrinter/SourceInterleave.cpp
38

If we're compiling source code using clang, we already have the source code in a buffer in memory. Re-reading it from the disk is useless work at best, and might not even work in some cases (for example, clang supports reading C code from stdin).

hfinkel added inline comments.
include/llvm/MC/MCTargetOptions.h
29

As I mentioned in the Clang review; this should be some kind of factored-out feature.

lib/CodeGen/AsmPrinter/SourceInterleave.cpp
129

I'd like to see this at greater than line granularity when column information is available. We should be able to have something like this:

// for (int i = 0; i < n; ++i)
//         ^
xor r1, r1, r1

...

// for (int i = 0; i < n; ++i)
//                         ^
cmp r1, r22

Doing this well means figuring out beforehand whether there are instructions with different column values for a particular line. This is a bit more work, but I can say from my experience making llvm-opt-report, it is definitely worth it.

Also, I'd like to see the line numbers prepended to the line. Since the lines don't always appear in order, this is important. Same with file names, when we print a line in a different file from the previously-printed line, we should say so:

// In foo/bar.c
// int x = a + b;
add r4, r5, r6
hfinkel added inline comments.Mar 14 2017, 1:42 PM
lib/CodeGen/AsmPrinter/SourceInterleave.cpp
38

I think that it is definitely worth considering about more of a handshake here between the frontend and the backend.

That having been said, we have the same problem with stdin code as we do with optimization reports. We just can't support that in a reasonable way right now.

emaste added a subscriber: emaste.Jun 11 2017, 5:36 AM
MatzeB resigned from this revision.Aug 15 2017, 11:09 AM