Page MenuHomePhabricator

Revert D68041 "[PGO] Don't group COMDAT variables for compiler generated profile variables in ELF"
AbandonedPublic

Authored by MaskRay on Jul 28 2020, 9:36 AM.

Details

Summary

D68041 placed __profc_, __profd_ and (if exists) __profvp_ in different comdat groups.
There are some issues:

  • Cost: one or two additional section headers (.group section(s)): 64 or 128 bytes on ELF64.
  • __profc_, __profd_ and (if exists) __profvp_ should be retained or discarded. Placing them into separate comdat groups is conceptually inferior.
  • If the prevailing group does not include __profvp_ (value profiling not used) but a non-prevailing group from another translation unit has __profvp_ (the function is inlined into another and triggers value profiling), there will be a stray __profvp_ if --gc-sections is not enabled.

We are currently investigating a mysterious PGO timeout. When things get
stabalized, we can do D84723, which can optimize more than D68041 but avoid the issues.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 28 2020, 9:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 28 2020, 9:36 AM
MaskRay requested review of this revision.Jul 28 2020, 9:36 AM

thanks. Can you update the summary of the revert patch -- I think the first paragraph does not belong.

In the summary, just list the issues of not sharing comdat group for profxx vars and how putting them in the same group solve it.

MaskRay edited the summary of this revision. (Show Details)Jul 28 2020, 10:02 AM

Cost: one or two additional section headers (.group section(s)): 64 or 128 bytes on ELF64.

Yes, a good reason.

profc_, profd_ and (if exists) __profvp_ should be retained or discarded. Placing them into separate comdat groups is conceptually inferior.

ok as reason -- though with the current linker behavior (using link order), if exist, they are retained or discarded together essentially. However if the prevailing module does not define profvp, we will end up with an unused profvp if GC is not on.

If the prevailing group does not include profvp_ (value profiling not used) but a non-prevailing group from another translation unit has profvp_ (the function is inlined into another and triggers value profiling), there will be a stray __profvp_.

Not sure I understand the stray profvp part -- profvp is not directly accessed by user function (but indirectly) -- so the __profvp in this case will be GCed.

MaskRay edited the summary of this revision. (Show Details)Jul 28 2020, 10:30 AM

Cost: one or two additional section headers (.group section(s)): 64 or 128 bytes on ELF64.

Yes, a good reason.

profc_, profd_ and (if exists) __profvp_ should be retained or discarded. Placing them into separate comdat groups is conceptually inferior.

ok as reason -- though with the current linker behavior (using link order), if exist, they are retained or discarded together essentially. However if the prevailing module does not define profvp, we will end up with an unused profvp if GC is not on.

If the prevailing group does not include profvp_ (value profiling not used) but a non-prevailing group from another translation unit has profvp_ (the function is inlined into another and triggers value profiling), there will be a stray __profvp_.

Not sure I understand the stray profvp part -- profvp is not directly accessed by user function (but indirectly) -- so the __profvp in this case will be GCed.

Updated: there will be a stray __profvp_ if --gc-sections is not enabled.

I mentioned this in another comment but did not state this in this description.

davidxl accepted this revision.Jul 28 2020, 10:34 AM

lgtm.

(probably better hold on the patch until after the timeout issue is resolved)

This revision is now accepted and ready to land.Jul 28 2020, 10:34 AM
MaskRay abandoned this revision.Aug 3 2020, 8:40 PM

We will just do D84723