Page MenuHomePhabricator

Convenience function for MCContext to get named sections when lowering.
ClosedPublic

Authored by dberris on Jun 26 2016, 8:56 PM.

Details

Summary

This change allows us to create uniquely identified "COMDAT" or "ELF
Group" sections while lowering. In particular, for ELF sections this is
useful for creating function-specific groups that get merged into the
same named section.

Also use const Twine& instead of StringRef for the getELF functions
while we're here.

Diff Detail

Event Timeline

dberris updated this revision to Diff 61928.Jun 26 2016, 8:56 PM
dberris retitled this revision from to Convenience function for MCStreamer to get named sections when lowering..
dberris updated this object.
dberris added reviewers: echristo, grosbach.
dberris added a subscriber: llvm-commits.

Reid, didn't you add something like this for COFF?

Sent from my phone

dberris added a reviewer: rnk.Jun 27 2016, 9:31 PM
echristo edited edge metadata.Jun 28 2016, 7:58 PM

My worry from looking at it briefly is that most of the uses of SHF_GROUP have additional flags that they want or'd into the section. Do you have an example refactor for the existing cases that we can use here?

Thanks!

-eric

I spent some time trying to find existing cases where this might be helpful, but I've yet to find suitable ones. Granted I've only looked at lib/Target/X86/*.

Most of the cases where this comes up are pretty straight-forward, using functions from TargetLoweringObjectFile to get the section for globals, etc. -- special section creation isn't very well supported through that interface either.

Extracting the flags might be doable but bringing ELF'isms out when lowering might be a little much.

Any suggestions on where else to look for cases where this interface (or an evolution of it) might be useful?

I've found the answer -- it seems this could be useful in TargetLoweringObjectFile and the various implementations there.

Let me update this to refactor some of the insides of TargetLoweringObjectFile to use this new interface.

dberris updated this revision to Diff 62181.Jun 28 2016, 11:02 PM
dberris edited edge metadata.
  • Refactor one call in TargetLoweringObjectFileImpl to use new API
rnk edited edge metadata.Jun 29 2016, 9:36 AM

Reid, didn't you add something like this for COFF?

MCContext::getAssociativeCOFFSection, which doesn't do this name appending stuff because it wastes object file space.

This patch is NFC, and that's fine, but at a high level, should we continue to name our sections things like .text._Z3foov (ala GCC), or should we be shortening to just .text? I thought Rafael did that at one point?

include/llvm/MC/MCStreamer.h
767

From reading the doc comments, this looks very ELF-specific. Maybe this should be getELFSectionInGroup? See a similar method getAssociativeCOFFSection.

rafael added inline comments.
lib/MC/MCAsmStreamer.cpp
1576 ↗(On Diff #62181)

I don't think this should be on the streamer, as the ELF and Asm implementations would be identical. This is just a convenience functions and should be implemented once in MCContext.

rnk added inline comments.Jun 29 2016, 11:31 AM
lib/MC/MCAsmStreamer.cpp
1576 ↗(On Diff #62181)

+1, it would live along with the other getELF*, getCOFF* etc methods. The XData and PData stuff lives here for bad reasons.

dberris updated this revision to Diff 62324.Jun 29 2016, 6:02 PM
dberris edited edge metadata.
  • Move API from MCStreamer to MCContext
dberris marked 2 inline comments as done.Jun 29 2016, 6:04 PM

Thanks -- I've moved this to MCContext instead.

include/llvm/MC/MCStreamer.h
767

Correct -- renamed to 'getELFNamedSection' instead?

dberris retitled this revision from Convenience function for MCStreamer to get named sections when lowering. to Convenience function for MCContext to get named sections when lowering..Jun 29 2016, 6:09 PM

Much better. It should at least rename Section and Group to just Prefix and Suffix, since there nothing specific about groups.

Having mentioned that, would it be too expensive to make the existing getELFSection overloads take a Twine instead of a StringRef?

dberris updated this revision to Diff 62326.Jun 29 2016, 6:47 PM
  • Rename "Section" -> "Prefix", "Group" -> "Suffix"

I'm not too sure about the change to use Twine instead of StringRef, but I'm happy to do do it as a separate change to see how expensive it could become.

Actually, having read a little about Twine, I'm convinced it's worth doing. I'll work on refactoring the other "getELF*" functions at least to use Twine instead of StringRef.

dberris updated this revision to Diff 62335.Jun 29 2016, 9:35 PM
  • Refactor getELF* functions to use const Twine& instead of StringRef
dberris updated this object.Jun 29 2016, 9:37 PM

Suggested refactoring to use Twine instead of StringRef now done.

rafael accepted this revision.Jun 30 2016, 5:42 AM
rafael added a reviewer: rafael.

LGTM. If the calls to str() are ever a problem we can replace them with the slightly more verbose toStringRef.

This revision is now accepted and ready to land.Jun 30 2016, 5:42 AM

Thanks Rafael -- I don't have commit powers, anybody care to land on my behalf?

Thanks in advance!

This revision was automatically updated to reflect the committed changes.

Committed thusly:

M lib/MC/MCContext.cpp
M lib/CodeGen/TargetLoweringObjectFileImpl.cpp
M include/llvm/MC/MCContext.h
r274336 = e2f8e9e22b5937d691814657012bc0997a3380a4
(refs/remotes/origin/master)

-eric