-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.
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?
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?
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.
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 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.
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.
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.
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.
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?