fdebug-info-for-profiling flag unexpectedly affects code generation

[jmorse]

Here's a godbolt link showing a difference in codegen produced by -fdebug-info-for-profiling: https://godbolt.org/z/sx8TEdPcb . An extra/spurious xor appears with the flag, and the identity of one of the functions changes. As far as I'm aware, -fdebug-info-for-profiling is intended to affect only the production of debugging information, not the generation of code to make it more amenable to debugging or profiling, which would make this a bug.

The (heavily reduced sorry) reproducer, for completeness: (compile it with clang -O2 -g -c, and then add/remove -fdebug-info-for-profiling).

struct str {
  str(char *);
};
using str2 = str;
struct foo {
  foo(str2);
};
struct bar {
  struct fail : foo {
    char msg;
    fail() : foo(&msg) {}
  };
  void clear() { fail(); }
};
struct baz : bar {
  void set(bool) { clear(); }
};
baz operator<<(baz _a, char) { _a.set(0); }

I dug into why this happens, and it comes in three stages:

• The Inliner has an EnableDeferral flag that makes the inliner less aggressive for certain static functions, so that they're small enough to be entirely inlined and eliminated from a file,
• This flag is enabled during pass-construction if PGO options are enabled
• PGO options are enabled if -fdebug-info-for-profiling is enabled

Thus, through a chain of decisions, we end up with different inlining choices because of debugging information. The most suspicious to me is the last, enabling PGO options if profiling debug-info is desired. This was added in c76a27e to ensure that the add-discriminator pass was enabled with the -fdebug-info-for-profiling. I would imagine that we need to directly write that logic somewhere, rather than using PGOOptions as a proxy.

[llvmbot]

@llvm/issue-subscribers-debuginfo

Author: Jeremy Morse (jmorse)

Details

Here's a godbolt link showing a difference in codegen produced by `-fdebug-info-for-profiling`: https://godbolt.org/z/sx8TEdPcb . An extra/spurious `xor` appears with the flag, and the identity of one of the functions changes. As far as I'm aware, `-fdebug-info-for-profiling` is intended to affect only the production of debugging information, not the generation of code to make it more amenable to debugging or profiling, which would make this a bug.

The (heavily reduced sorry) reproducer, for completeness: (compile it with clang -O2 -g -c, and then add/remove -fdebug-info-for-profiling).

struct str {
  str(char *);
};
using str2 = str;
struct foo {
  foo(str2);
};
struct bar {
  struct fail : foo {
    char msg;
    fail() : foo(&msg) {}
  };
  void clear() { fail(); }
};
struct baz : bar {
  void set(bool) { clear(); }
};
baz operator<<(baz _a, char) { _a.set(0); }

I dug into why this happens, and it comes in three stages:

• The Inliner has an EnableDeferral flag that makes the inliner less aggressive for certain static functions, so that they're small enough to be entirely inlined and eliminated from a file,
• This flag is enabled during pass-construction if PGO options are enabled
• PGO options are enabled if -fdebug-info-for-profiling is enabled

Thus, through a chain of decisions, we end up with different inlining choices because of debugging information. The most suspicious to me is the last, enabling PGO options if profiling debug-info is desired. This was added in c76a27e to ensure that the add-discriminator pass was enabled with the -fdebug-info-for-profiling. I would imagine that we need to directly write that logic somewhere, rather than using PGOOptions as a proxy.

[dwblaikie]

(@rnk) Ah, thanks for noticing/filing/investigating... yeah, that seems unfortunate/wrong.

• The Inliner has an EnableDeferral flag that makes the inliner less aggressive for certain static functions, so that they're small enough to be entirely inlined and eliminated from a file,
• This flag is enabled during pass-construction if PGO options are enabled
• PGO options are enabled if -fdebug-info-for-profiling is enabled

This last one seems pretty questionable, but I guess it could be any one/combination of these steps that's ultimately to "Blame".

[rnk]

I think it's worth raising the question of, is -fdebug-info-for-profiling a debug info flag? I haven't talked to PGO folks (cc @amharc @snehasish ), but I suspect that the actual meaning of this flag is closer to: "build the program and prepare for sample PGO recompilation and optimization".

All PGO varieties attempt to replay the optimization decisions made up until mapping the profile data onto the IR, and that includes making the same inliner decisions, so this inliner tweak is probably part of that. You can find other examples of this, where we have additional inlining passes which effectively force the inliner to replay past inlining decisions, although I'm sure the ground truth here shifts over time.

Regardless of whether we change what -fdebug-info-for-profiling does, Clang should have orthogonal flags to control the debug info settings and the PGO pass pipeline setup.

[jmorse]

The original discussion conceived of it as -gprofile and being a strict super-set of the debug-info produced at -g:

https://discourse.llvm.org/t/rfc-encoding-code-duplication-factor-in-discriminator/42940/35?u=jmorse

Of course, things change over time: our interest here is that we configure -g to enable -fdebug-info-for-profiling by default, so that customers can debug and get good profiles with "normal" builds, but can turn it off if they're up against a file-size wall. But that then means there's a bunch of "-g affects codegen" issues.

Whichever way the options gets interpreted we'd still like to be able to keep debug-info decisions separate from codegen ones, even if -debug-info-for-profiling is being interpreted as both a debug-info and codegen flag.

[dwblaikie]

@jmorse thanks for the context on your side - good to know!

Yeah, given the original intent and flag naming, I'd tend towards preserving the original intent of this flag and introducing some other for the combined "do both debug and codegen affecting things that help sample based profiling".