This is an archive of the discontinued LLVM Phabricator instance.

CodeGen: Stick constant pool entries in COMDAT sections for WinCOFF
ClosedPublic

Authored by majnemer on Jul 12 2014, 11:20 PM.

Details

Summary

COFF lacks a feature that other object file formats support: mergeable
sections.

To work around this, MSVC sticks constant pool entries in special COMDAT
sections so that each constant is in it's own section. This permits
unused constants to be dropped and it also allows duplicate constants in
different translation units to get merged together.

This fixes PR20262.

Diff Detail

Repository
rL LLVM

Event Timeline

majnemer updated this revision to Diff 11344.Jul 12 2014, 11:20 PM
majnemer retitled this revision from to CodeGen: Stick constant pool entries in COMDAT sections for WinCOFF.
majnemer updated this object.
majnemer added reviewers: rafael, rnk, grosbach, echristo.
majnemer added a subscriber: Unknown Object (MLST).
grosbach requested changes to this revision.Jul 14 2014, 11:06 AM
grosbach edited edge metadata.

No feedback on the Windows specific aspects, as I don't know how that stuff works well enough to comment.

Some general comments inline. A couple of cleanup changes that should be split out into separate patches and an API cleanup to get better layering.

include/llvm/ADT/StringExtras.h
56 ↗(On Diff #11344)

These changes are distinct and should be a separate patch.

Also, documentation comment needs updated to match.

include/llvm/CodeGen/MachineConstantPool.h
124 ↗(On Diff #11344)

Moving this to a helper function makes sense, but should be a separate patch.

It should take a datalayout argument, too, not the target machine.

include/llvm/CodeGen/TargetLoweringObjectFileImpl.h
47 ↗(On Diff #11344)

Having these take an MCStreamer reference is really awkward. If that's necessary, it implies something isn't right about the layering elsewhere.

lib/Target/X86/X86TargetObjectFile.cpp
166 ↗(On Diff #11344)

OK, here's the layering issue. These functions should be strictly lookup, but here we're mutating things by calling an output function on the streamer. This function isn't the right place for figuring out the symbol names and emitting them to the streamer. GetCPISymbol() is a better place for that.

This revision now requires changes to proceed.Jul 14 2014, 11:06 AM
majnemer updated this revision to Diff 11406.Jul 14 2014, 3:30 PM
majnemer edited edge metadata.

Address review comments.

majnemer added inline comments.Jul 14 2014, 3:31 PM
include/llvm/ADT/StringExtras.h
56 ↗(On Diff #11344)

Done.

include/llvm/CodeGen/MachineConstantPool.h
124 ↗(On Diff #11344)

Done.

include/llvm/CodeGen/TargetLoweringObjectFileImpl.h
47 ↗(On Diff #11344)

Fixed.

lib/Target/X86/X86TargetObjectFile.cpp
166 ↗(On Diff #11344)

Fixed.

grosbach accepted this revision.Jul 14 2014, 3:39 PM
grosbach edited edge metadata.

Rather terrifying how MSVC does this stuff, but patch LGTM.

Minor nit inline.

lib/Target/X86/X86TargetObjectFile.cpp
165 ↗(On Diff #11406)

"Sym" is unused.

I think you can just directly do:

return getContext().getCOFFSection(".rdata", Characteristics, Kind, COMDATSymName, COFF::IMAGE_COMDAT_SELECT_ANY));
This revision is now accepted and ready to land.Jul 14 2014, 3:39 PM
majnemer closed this revision.Jul 14 2014, 4:06 PM
majnemer updated this revision to Diff 11407.

Closed by commit rL213006 (authored by @majnemer).