Page MenuHomePhabricator

Use .text.unlikely and .text.eh prefixes for MachineBasicBlock sections.
ClosedPublic

Authored by snehasish on Apr 23 2020, 11:59 AM.

Details

Summary

Instead of adding a ".unlikely" or ".eh" suffix for machine basic blocks,
this change updates the behaviour to use an appropriate prefix
instead. This allows lld to group basic block sections together
when -z,keep-text-section-prefix is specified and matches the behaviour
observed in gcc.

Diff Detail

Event Timeline

snehasish created this revision.Apr 23 2020, 11:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2020, 11:59 AM
tmsriram added inline comments.Apr 23 2020, 2:07 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
889

This looks good. This should also be done for ".eh", .text.eh must be the prefix. Further, the parent name must only be appended for unlikely and eh sections as there is only one such section for each function and using the function name guarantees that.

For other sections, there is no need to append the parent name. Either it should be .text (or .text.hot) or .text.<MBBSymbolName> when UniqueBBSectionNames is used. Does this make sense? Thanks for working on this.

snehasish retitled this revision from Use .text.unlikely prefix for cold MachineBasicBlock sections. to Use .text.unlikely and .text.eh prefixes for MachineBasicBlock sections..Apr 23 2020, 3:17 PM
snehasish edited the summary of this revision. (Show Details)
snehasish updated this revision to Diff 259740.Apr 23 2020, 3:36 PM

Use a ".text.eh" prefix instead of ".eh" suffix for exception machine basic block sections.
Update the basic-block-sections-clusters-eh test.

snehasish marked an inline comment as done.Apr 23 2020, 3:41 PM
snehasish added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
889

Done. I've updated the diff to use the ".text.eh" prefix as suggested.

The behaviour for the other sections is actually unchanged. We use the name of the parent section and not the parent. I just removed the static_cast to MCSectionELF* before calling getName() since it is unnecessary.

tmsriram accepted this revision.Apr 23 2020, 6:10 PM
tmsriram added a reviewer: efriedma.

This is a small change and looks good to me. I think this is consistent with what the linker expects for unlikely sections, so I am approving it. Adding @efriedma just to be sure. Please wait for a response before pushing.

This revision is now accepted and ready to land.Apr 23 2020, 6:12 PM
tmsriram added inline comments.Apr 23 2020, 6:13 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
881

Nit, use braces around the else too.

snehasish updated this revision to Diff 259784.Apr 23 2020, 7:06 PM

Addressed review comment.

snehasish marked an inline comment as done.Apr 23 2020, 7:07 PM
efriedma accepted this revision.Apr 24 2020, 1:43 PM

LGTM. It looks like gcc also emits .text.unlikely.foo

snehasish edited the summary of this revision. (Show Details)Apr 24 2020, 2:19 PM
snehasish added a subscriber: eli.friedman.

Thanks for the reviews @eli.friedman @tmsriram.
Can someone go ahead and push this this for me since I don't have commit access?

This revision was automatically updated to reflect the committed changes.