This is an archive of the discontinued LLVM Phabricator instance.

Invoke add-discriminator at -g0 -fsample-profile
ClosedPublic

Authored by danielcdh on Sep 30 2016, 2:45 PM.

Details

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.

Event Timeline

danielcdh updated this revision to Diff 73147.Sep 30 2016, 2:45 PM
danielcdh retitled this revision from to Invoke add-discriminator at -g0 -fsample-profile.
danielcdh updated this object.
danielcdh added reviewers: davidxl, dnovillo.
danielcdh added a subscriber: llvm-commits.
dnovillo added inline comments.Oct 3 2016, 7:00 AM
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 added inline comments.Oct 3 2016, 7:09 AM
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?

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

dnovillo edited edge metadata.Oct 3 2016, 8:27 AM

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 updated this revision to Diff 73298.Oct 3 2016, 10:04 AM
danielcdh edited edge metadata.

update

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

danielcdh updated this revision to Diff 73299.Oct 3 2016, 10:07 AM

revert the flag change.

probinson added inline comments.
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 added inline comments.
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 added inline comments.Oct 4 2016, 6:20 PM
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 added inline comments.Oct 4 2016, 6:51 PM
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 edited edge metadata.Oct 5 2016, 6:06 PM

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

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.

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 updated this revision to Diff 73929.Oct 7 2016, 7:03 AM
danielcdh edited edge metadata.

Invoke add-discriminator regardless of -fsample-profile.

dnovillo accepted this revision.Oct 7 2016, 7:15 AM
dnovillo edited edge metadata.

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.

This revision is now accepted and ready to land.Oct 7 2016, 7:15 AM

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?

danielcdh closed this revision.Oct 7 2016, 8:30 AM