Invoke add-discriminator at -g0 -fsample-profile

[danielcdh]

Summary

-fsample-profile needs discriminator, which will not be added if built with -g0. This patch makes sure the discriminator is added for sample-profile at -g0. A followup patch will be send out to update clang tests.

[dnovillo]

lib/Transforms/Utils/AddDiscriminators.cpp

165

Is this the right check to perform? This check is needed to determine whether DWARF support exists for discriminators. How is that related to -g0?

[danielcdh]

lib/Transforms/Utils/AddDiscriminators.cpp

165

With -g0, getDwarfVersion() will return 0. I think it's fine to have discriminators even when the debug info does not support it. It will simply not getting emitted?

[danielcdh]

Another option would be: make SampleProfileFile option global, and checks if that option is empty here to decide if discriminator pass is needed.

[dnovillo]

In D25132#559074, @danielcdh wrote:

Another option would be: make SampleProfileFile option global, and checks if that option is empty here to decide if discriminator pass is needed.

Yeah, I think this might be a safer option. Is there no other way to decide whether -g0 was passed? Isn't -g0 recorded as a debug level or something like that?

lib/Transforms/Utils/AddDiscriminators.cpp

165

Sure, but it will also trigger when the target does not support DWARF4. I think, this would cause a failure for DWARF <4, but Eric or David would be better at answering this.

[danielcdh]

update

[danielcdh]

I've updated the patch to check if samplepgo is enabled, and then decided if add-discriminator pass is mandatory.

[danielcdh]

revert the flag change.

[probinson]

lib/Transforms/Utils/AddDiscriminators.cpp

165

What happens if you specify -fsample-profile -gdwarf-3? Do you get a DWARF 4 line table with discriminators even though you said you wanted DWARF 3? I don't think that's a great idea.
I think it's fine to emit a DWARF 4 line table with discriminators if you see -fsample-profile and no debug info (-g0 or no -g at all), but if somebody asked for DWARF 2 or 3 then you should respect that.
If you want to tell the user that -fsample-profile requires DWARF 4, you should do that in the Clang driver, not simply override their choice here.

[danielcdh]

update

[danielcdh]

lib/Transforms/Utils/AddDiscriminators.cpp

165

I agree that discriminator should *not* be emitted when user specifies -gdwarf-3. But user should still be able to use -fsample-profile if they add -gdwarf-3. How about we assign discriminator early (kind of like canonicalization). But when generating code, we only emit discriminator when dwarf version is >= 4?

I've updated the patch to reflect the change.

[probinson]

lib/Transforms/Utils/AddDiscriminators.cpp

165

Why analyze the CFG to create discriminators that you are not going to emit? What value does that have? Seems like a waste of time.

[danielcdh]

lib/Transforms/Utils/AddDiscriminators.cpp

165

For SamplePGO, it uses debug info including discriminator to annotate CFG. So even when the discriminator is not emitted, it needs to be used by the profile annotator during compilation.

[echristo]

Hi Dehao,

I think if we're not going to emit them for dwarf < 4 then we don't need to plumb the sample pgo bit through the pass no? Just unconditionally add them if requested as an analysis for sample pgo to use? I'm assuming the pass is only enabled when we're either a) emitting debug info, or b) going to annotate IR for samplepgo.

Thoughts?

-eric

[probinson]

In D25132#562973, @echristo wrote:

I'm assuming the pass is only enabled when we're either a) emitting debug info, or b) going to annotate IR for samplepgo.

Actually it looks like Clang unconditionally adds the pass and then lets the pass decide whether to do anything on its own. I don't offhand know where LTO decides what to use for a pass pipeline.

[echristo]

In D25132#562990, @probinson wrote:

In D25132#562973, @echristo wrote:

I'm assuming the pass is only enabled when we're either a) emitting debug info, or b) going to annotate IR for samplepgo.

Actually it looks like Clang unconditionally adds the pass and then lets the pass decide whether to do anything on its own. I don't offhand know where LTO decides what to use for a pass pipeline.

Sure, it seems reasonable to have it added unconditionally either way for optimization purposes.

-eric

[danielcdh]

Invoke add-discriminator regardless of -fsample-profile.

[dnovillo]

Thanks, Dehao. This LGTM now. I'm not 100% sure about the code paths in *Dwarf*.cpp, but they look safe enough. The only concern there would be any other code that also calls getDiscriminator, in which case, perhaps we could sink the DWARF version check into getDiscriminator and have getDiscriminator return ErrorOr.

[danielcdh]

In D25132#564464, @dnovillo wrote:

Thanks, Dehao. This LGTM now. I'm not 100% sure about the code paths in *Dwarf*.cpp, but they look safe enough. The only concern there would be any other code that also calls getDiscriminator, in which case, perhaps we could sink the DWARF version check into getDiscriminator and have getDiscriminator return ErrorOr.

Thanks for the reviews.

I grepped around the code base, looks like SampleProfile.cpp is the only transformation/analysis that uses getDiscriminator. Other callsites are only for output processing.

This patch needs to get in after https://reviews.llvm.org/D25133 is committed too ensure no test breakage. Could you help take a look at that too?

lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

DIE *DwarfCompileUnit::constructInlinedScopeDIE(LexicalScope *Scope) {
 
   attachRangesOrLowHighPC(*ScopeDIE, Scope->getRanges());
 
   // Add the call site information to the DIE.
   const DILocation *IA = Scope->getInlinedAt();
   addUInt(*ScopeDIE, dwarf::DW_AT_call_file, None,
           getOrCreateSourceID(IA->getFilename(), IA->getDirectory()));
   addUInt(*ScopeDIE, dwarf::DW_AT_call_line, None, IA->getLine());
-  if (IA->getDiscriminator())
+  if (IA->getDiscriminator() && DD->getDwarfVersion() >= 4)
     addUInt(*ScopeDIE, dwarf::DW_AT_GNU_discriminator, None,
             IA->getDiscriminator());
 
   // Add name to the name table, we do this here because we're guaranteed
   // to have concrete versions of our DW_TAG_inlined_subprogram nodes.
   DD->addSubprogramNames(InlinedSP, *ScopeDIE);
 
   return ScopeDIE;

lib/CodeGen/AsmPrinter/DwarfDebug.cpp

void DwarfDebug::recordSourceLine(unsigned Line, unsigned Col, const MDNode *S,
   StringRef Fn;
   StringRef Dir;
   unsigned Src = 1;
   unsigned Discriminator = 0;
   if (auto *Scope = cast_or_null<DIScope>(S)) {
     Fn = Scope->getFilename();
     Dir = Scope->getDirectory();
     if (auto *LBF = dyn_cast<DILexicalBlockFile>(Scope))
+      if (DwarfVersion >= 4)
         Discriminator = LBF->getDiscriminator();
 
     unsigned CUID = Asm->OutStreamer->getContext().getDwarfCompileUnitID();
     Src = static_cast<DwarfCompileUnit &>(*InfoHolder.getUnits()[CUID])
               .getOrCreateSourceID(Fn, Dir);
   }
   Asm->OutStreamer->EmitDwarfLocDirective(Src, Line, Col, Flags, 0,
                                           Discriminator, Fn);
 }

lib/MC/MCDwarf.cpp

if (FileNum != LineEntry.getFileNum()) {
       MCOS->EmitIntValue(dwarf::DW_LNS_set_file, 1);
       MCOS->EmitULEB128IntValue(FileNum);
     }
     if (Column != LineEntry.getColumn()) {
       Column = LineEntry.getColumn();
       MCOS->EmitIntValue(dwarf::DW_LNS_set_column, 1);
       MCOS->EmitULEB128IntValue(Column);
     }
-    if (Discriminator != LineEntry.getDiscriminator()) {
+    if (Discriminator != LineEntry.getDiscriminator() &&
+        MCOS->getContext().getDwarfVersion() >= 4) {
       Discriminator = LineEntry.getDiscriminator();
       unsigned Size = getULEB128Size(Discriminator);
       MCOS->EmitIntValue(dwarf::DW_LNS_extended_op, 1);
       MCOS->EmitULEB128IntValue(Size + 1);
       MCOS->EmitIntValue(dwarf::DW_LNE_set_discriminator, 1);
       MCOS->EmitULEB128IntValue(Discriminator);
     }
     if (Isa != LineEntry.getIsa()) {

lib/Transforms/Utils/AddDiscriminators.cpp

 /// location as instruction I2 of successor B2, then it creates a new
 /// lexical block for I2 and all the instruction in B2 that share the same
 /// file and line location as I2. This new lexical block will have a
 /// different discriminator number than I1.
 static bool addDiscriminators(Function &F) {
   // If the function has debug information, but the user has disabled
   // discriminators, do nothing.
   // Simlarly, if the function has no debug info, do nothing.
-  // Finally, if this module is built with dwarf versions earlier than 4,
-  // do nothing (discriminator support is a DWARF 4 feature).
-  if (NoDiscriminators || !F.getSubprogram() ||
-      F.getParent()->getDwarfVersion() < 4)
+  if (NoDiscriminators || !F.getSubprogram())
     return false;
 
   bool Changed = false;
   Module *M = F.getParent();
   LLVMContext &Ctx = M->getContext();
   DIBuilder Builder(*M, /*AllowUnresolved*/ false);
 
   typedef std::pair<StringRef, unsigned> Location;