Page MenuHomePhabricator

Change debug-info-for-profiling from a TargetOption to a function attribute.
ClosedPublic

Authored by danielcdh on Jan 26 2017, 3:58 PM.

Event Timeline

danielcdh created this revision.Jan 26 2017, 3:58 PM
ahatanak added inline comments.
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

Is debug-info-for-profiling used to turn on and off debug info for profiling on a per-function basis? I'm not very familiar with this function, but it looks like HasDebugInfoForProfiling is initialized to false in the constructor and then it will always be true after beginFunction is called on a function that has attribute debug-info-for-profiling=true.

mehdi_amini added inline comments.Jan 26 2017, 5:26 PM
include/llvm/IR/Attributes.td
182 ↗(On Diff #85982)

That'll be long in the IR, where there is already a long list, what about "prof-dbg-info" or something like that that would make sense?

lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

Yep that seems broken to me. If foo has the attribute and not bar, we'll emit the profiling debug info for bar if foo is processed first.

danielcdh added inline comments.Jan 26 2017, 5:38 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

The issue is that I cannot check for function's attributes in DwarfUnit::applySubprogramAttributes, any suggestions on how I can pass down this attribute to that function?

mehdi_amini added inline comments.Jan 26 2017, 5:44 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

Why not do here HasDebugInfoForProfiling = MF->getFunction()->debugInfoForProfiling(); ?

Is there a reason to make this a function attribute rather than a flag carried in the metadata? Having something related to debug info as an attribute seems odd.

test/DebugInfo/Inputs/gmlt_profiling.ll
5 ↗(On Diff #85982)

f2() does not appear in any checks and is not used anywhere else in the test. Why is it here?

7 ↗(On Diff #85982)

f3() and f4() do not appear in any checks, why are they here?

14 ↗(On Diff #85982)

Writing checks for dwarfdump output is tricky because you're really trying to match several attribute lines within the same DWARF entry. So here you want something like this:

CHECK: DW_TAG_subprogram
CHECK-NOT: {{DW_TAG|NULL}}
CHECK: DW_AT_name {{.*}} "f1"
CHECK-NEXT: DW_AT_decl_file
CHECK-NEXT: DW_AT_decl_line

This assumes that name/file/line appear in that exact sequence, but there are lots of debug-info tests with that kind of assumption. Unless/until we get a fancier debug-info matcher this is the kind of thing we have to do.

43 ↗(On Diff #85982)

Generally we encourage people to remove attributes that aren't relevant or necessary to the test. Here, probably only "debug-info-for-profiling" is the relevant one? Removing the others helps clarify what the test is about.

danielcdh added inline comments.Jan 26 2017, 6:00 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

Because the llvm::DwarfDebug::beginFunction is called first before llvm::DwarfUnit::applySubprogramAttributes are called.

E.g. you have f1, f2, f3

the call sequence is:
beginFunction(f1)
beginFunction(f2)
beginFunction(f3)
applySubprogramAttributes(subprogramof_f1)
applySubprogramAttributes(subprogramof_f2)
applySubprogramAttributes(subprogramof_f3)

The current logic is, if there is any function that has HasDebugInfoForProfiling=true, then all function will have it.

probinson added inline comments.Jan 26 2017, 6:07 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

All the more reason to put it at module or CU level, rather than function level.

mehdi_amini added inline comments.Jan 26 2017, 6:26 PM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

If the behavior you want is to have it enabled for the module, then yes like @probinson says a module level construct would make more sense.

I don't enough about -gmlt to know what we would expect if we have a.cpp and b.cpp and we build with LTO one with -g and the other one with -gmlt ?

danielcdh updated this revision to Diff 86001.Jan 26 2017, 7:18 PM
danielcdh marked 3 inline comments as done.

change to use module flags.

danielcdh added inline comments.Jan 27 2017, 8:07 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

Updated to use module flag for this.

test/DebugInfo/Inputs/gmlt_profiling.ll
14 ↗(On Diff #85982)

Done, but the CHECK-NOT cannot be used because we have DW_AT_low_pc and DW_AT_high_pc in between.

mehdi_amini added inline comments.Jan 27 2017, 8:15 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

Can you clarify what is the behavior in this case:

what we would expect if we have a.cpp and b.cpp and we build with LTO one with -g and the other one with -gmlt ?

danielcdh added inline comments.Jan 27 2017, 8:29 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

I am not debug info expert, Eric may have more accurate answer.

My understanding is that one will have more debug info emitted, the other will only have line table emitted. What will happen in LTO?

mehdi_amini added inline comments.Jan 27 2017, 8:32 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

I'm asking because I haven ming that of DebugInfoForProfiling will be enabled by -gmlt, but that may not be true? If so when/how is DebugInfoForProfiling set?

danielcdh added inline comments.Jan 27 2017, 8:45 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

It will be set with -fdebug-info-for-profiling.

A follow-up cfe patch is in: https://reviews.llvm.org/D29205

mehdi_amini added inline comments.Jan 27 2017, 8:56 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

Ok so back to my question:

what we would expect if we have a.cpp and b.cpp and we build with LTO one with -g and the other one with -fdebug-info-for-profiling? How does your module flag affect that?

danielcdh added inline comments.Jan 27 2017, 9:07 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

-fdebug-info-for-profiling is orthogonal with -g/-gmlt. So I guess the question should be:

what we would expect if we have a.cpp and b.cpp and we build with LTO, a.cpp with -fdebug-info-for-profiling and b.cpp without the flag? How does your module flag affect that?

The answer is

  • a.cpp will have the flag and thus all the logic guarded by this flag in backend will be executed.
  • b.cpp will not have it and will not have related code executed.
  • If some functions are imported from b.cpp to a.cpp, it will inherit the flag from a.cpp thus have related code executed.
  • If some functions are imported from a.cpp to c.cpp, it will inherit the flag from b.cpp thus not have related code executed.

I guess this also applies to the cases that a.cpp is built with -g and b.cpp is built with -gmlt.

mehdi_amini added inline comments.Jan 27 2017, 9:56 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

During Monolithic LTO, the two files are *merged*. There is a single CodeGen happening.

I'm still trying to figure out what is the impact of If some functions are imported from b.cpp to a.cpp, it will inherit the flag from a.cpp thus have related code executed, because I don't know enough about -fdebug-info-for-profiling.

I think I originally misread this comment:

// Under -gmlt, skip building the subprogram if there are no inlined
// subroutines inside it. But with -fdebug-info-for-profiling, the subprogram
// is still needed as we need its source location.

If I understand correctly, the module flag would make the debug info emission more conservative. Does it mean that adding -fdebug-info-for-profiling with -g does not have any effect on the Dwarf emission? It is only in effect with DICompileUnit::LineTablesOnly?

danielcdh added inline comments.Jan 27 2017, 10:04 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

-fdebug-info-for-profiling has the following implication:

  1. emit extra subprogram info (e.g. decl_line) even with -gmlt
  2. emit discriminator and encode optimization effects to discriminator.

For #1, -g also emits these extra info, thus it only applies to -gmlt
For #2, which is under review in https://reviews.llvm.org/D26420, it applies to both -g and -gmlt builds.

mehdi_amini accepted this revision.Jan 27 2017, 10:16 AM
mehdi_amini added inline comments.
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1136

Thanks for the explanation!
With this I'm fine with the module flag.

If @probinson does not have any other comment that LGTM.

This revision is now accepted and ready to land.Jan 27 2017, 10:16 AM

Regarding the mixed-mode LTO question, my understanding is that if we have a module-level flag then it would apply to everything in the merged module. The merged module will still have debug info organized per original compilation unit. So, if we have a per-CU flag and compiled flag-on.cpp and flag-off.cpp, then functions from flag-on.cpp would get the extra info and functions from flag-off.cpp would not, with the caveat that if you inline a function across compilation units, then the flag from the CU for the caller would apply.

I believe we have a module flag here, so the entire module would be treated as if -fdebug-info-for-profiling had been turned on for all CUs.

(Did I get that right?)

test/DebugInfo/Inputs/gmlt_profiling.ll
14 ↗(On Diff #85982)

The CHECK-NOT should not match a DW_AT line. You need it to ensure that the attributes belong to the same TAG that you found.

After a lengthy discussion with @dblaikie (Thanks David for the input), let me add some more context to this patch.

  • What is the impact of adding -fdebug-info-for-profiling
    • It will not affect the generated code
    • It will increase the debug info size in -gmlt by ~17%
    • It will not increase the debug info size in -g binary
    • The compile time change should be negligible
  • What we use a binary built with out -fdebug-info-for-profiling to collect profile
    • The profile will be inaccurate and will yield sub-optimal performance when used to drive SamplePGO
  • What is the motivating example that you want to compile some sources with -fdebug-info-for-profiling and some without
    • If the debug info size is a concern for -gmlt binary, then one would want to only enable this flag for sources that is performance-critical

Regarding the mixed-mode LTO question, my understanding is that if we have a module-level flag then it would apply to everything in the merged module. The merged module will still have debug info organized per original compilation unit. So, if we have a per-CU flag and compiled flag-on.cpp and flag-off.cpp, then functions from flag-on.cpp would get the extra info and functions from flag-off.cpp would not, with the caveat that if you inline a function across compilation units, then the flag from the CU for the caller would apply.

I believe we have a module flag here, so the entire module would be treated as if -fdebug-info-for-profiling had been turned on for all CUs.

I think you are right. If we always emit this module flag (with value 0 or 1), when there are some CUs built with this flag, and some CUs without, in LTO, it will emit a warning and turn this flag on for all CUs.

Dehao

(Did I get that right?)

  • What is the motivating example that you want to compile some sources with -fdebug-info-for-profiling and some without
    • If the debug info size is a concern for -gmlt binary, then one would want to only enable this flag for sources that is performance-critical

This is an argument *against* a module flag.

  • What is the motivating example that you want to compile some sources with -fdebug-info-for-profiling and some without
    • If the debug info size is a concern for -gmlt binary, then one would want to only enable this flag for sources that is performance-critical

This is an argument *against* a module flag.

Right ;-) I think I need to raise the point before close the patch.

But my opinion is that this use cases (if any) would be rare because:

  • 17% seem small, but people may have other opinions
  • adding per-CU flag is manual and tedious
  • per-CU flag may need to change as new source coming in/old source change that updates the hot CU distribution

That being said, I would think it's better to force this flag in LTO when mixed flag situation happens. But will let upstream decide if this would be a feature or a bug.

Thanks,
Dehao

danielcdh updated this revision to Diff 86117.Jan 27 2017, 2:02 PM

Change to use DICompileUnit to pass down the flag.

probinson added inline comments.Jan 27 2017, 2:17 PM
test/DebugInfo/Inputs/gmlt_profiling.ll
7 ↗(On Diff #86117)

The CHECK-NOT needs to go before DW_AT_name.
You are using CHECK-NEXT to guarantee that decl_file and decl_line are in the same tag as DW_AT_name.

dblaikie added inline comments.Jan 27 2017, 2:18 PM
include/llvm/IR/DebugInfoMetadata.h
1140–1142

I don't think this accessor is needed

lib/CodeGen/AsmPrinter/DwarfDebug.h
266–268 ↗(On Diff #86117)

Presumably this (& the accessor below) can be removed now?

lib/IR/Module.cpp
468–475 ↗(On Diff #86117)

Presumably this should go away now that it's in the CU metadata.

test/DebugInfo/Generic/gmlt.test
0

This line should probably be in gmlt_profiling.ll

0

This line should probably be in gmlt.ll (both gmlt.ll and gmlt_profiling.ll would move from test/DebugInfo/Inputs to test/DebugInfo/Generic)

danielcdh updated this revision to Diff 86131.Jan 27 2017, 2:50 PM
danielcdh marked 6 inline comments as done.

update also remove TargetOption.DebugInfoForProfiling.

I didn't see a binary bitcode upgrade testcase — could you please add one?

aprantl added inline comments.Jan 27 2017, 3:12 PM
include/llvm/IR/DIBuilder.h
107–110

Missing doxygen for the new(er) flag(s).

It might also be a good idea to document the purpose of the flag in SourceLevelDebugging.rst,

danielcdh updated this revision to Diff 86136.Jan 27 2017, 3:16 PM

Add bitcode test.

danielcdh updated this revision to Diff 86139.Jan 27 2017, 3:26 PM

update doxygen.

It might also be a good idea to document the purpose of the flag in SourceLevelDebugging.rst,

Could you help suggest which section I should add related doc? I think it would be good to add to the doc that introduces different debug level, but seems no such content exists in the current doc.

That document clearly needs a some love :-)
I would suggest adding a new Compile Unit section to either of the two Debugging information format sections (which should be merged) and adding a stub about global (compile-unit-wide) flags that influence the DWARF/CodeView debug info generator.

That document clearly needs a some love :-)
I would suggest adding a new Compile Unit section to either of the two Debugging information format sections (which should be merged) and adding a stub about global (compile-unit-wide) flags that influence the DWARF/CodeView debug info generator.

Sounds reasonable. But that seems quite some extra work that is outside the scope of this patch. Shall we start a separate thread to improve the documentation of the debug info?

Works for me.

Commented on bitcode test

test/Bitcode/upgrade-debug-info-for-profiling.ll
5

This probably isn't necessary.

14

Perhaps I'm confused about what this is testing*, but shouldn't the default be debugInfoForProfiling: false?

*) I was expecting a DICompilieUnit produced by today's trunk without a bitcode record for the new flag, so we can verify that the old bitcode format is updated correctly.

danielcdh updated this revision to Diff 86288.Jan 30 2017, 8:02 AM
danielcdh marked an inline comment as done.

update upgrading test

danielcdh added inline comments.Jan 30 2017, 8:02 AM
test/Bitcode/upgrade-debug-info-for-profiling.ll
14

Not sure if I understand what to test in this upgrade test.

What I did here:

  1. build the .ll file with the current trunk without this patch to produce the .bc file
  2. check "debugInfoForProfiling: true" does *not* exists in the llvm-dis output.

Am I doing the right thing?

aprantl accepted this revision.Feb 1 2017, 2:09 PM

I added one comment to the testcase, otherwise this is fine now.

test/Bitcode/upgrade-debug-info-for-profiling.ll
9

Ah I didn't see the -NOT.
It is better to write this as
CHECK: DICompileUnit(language: DW_LANG_C99, file: !{{[0-9]+]], emissionKind: FullDebug)
otherwise /bin/true would also pass this test.

danielcdh updated this revision to Diff 86716.Feb 1 2017, 2:18 PM
danielcdh marked an inline comment as done.

update

dblaikie accepted this revision.Feb 1 2017, 2:27 PM

Looks good

danielcdh updated this revision to Diff 86731.Feb 1 2017, 2:52 PM

update a test to point to the correct location.

danielcdh closed this revision.Feb 1 2017, 2:56 PM
dlj added a subscriber: dlj.Feb 1 2017, 4:56 PM
dlj added inline comments.
test/DebugInfo/Generic/gmlt.test
0

Moving this file to Generic/ means that other tests (notably, under X86/) now use files not within their Inputs, or within their parents' Inputs. Was that the intent of this comment?

This change was addressing one of the review comments.

test/DebugInfo/X86/gmlt.test do now depends on the input file from test/DebugInfo/Generic/gmlt.ll. If this is an issue, I can send another patch to move gmlt.ll back to test/DebugInfo/Input and add test/DebugInfo/Generic/gmlt.test back.

updated in r293852

I should have discovered the breaking x86 version of the test earlier...

Thanks for the reviews!