This is an archive of the discontinued LLVM Phabricator instance.

[LTO][MC] Discard non-prevailing defined symbols in module-level assembly
ClosedPublic

Authored by ychen on Mar 16 2021, 11:03 PM.

Details

Summary

This is the alternative approach to D96931.

In LTO, for each module with inlineasm block, prepend directive ".lto_discard <sym>, <sym>*" to the beginning of the inline
asm. ".lto_discard" is both a module inlineasm block marker and (optionally) provides a list of symbols to be discarded.

In MC while emitting for inlineasm, discard symbol binding & symbol
definitions according to ".lto_disard".

Diff Detail

Event Timeline

ychen created this revision.Mar 16 2021, 11:03 PM
ychen requested review of this revision.Mar 16 2021, 11:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2021, 11:03 PM
ychen edited the summary of this revision. (Show Details)Mar 16 2021, 11:08 PM
MaskRay added a comment.EditedMar 16 2021, 11:57 PM

Thanks for the patch. Will take a closer look, but at a glance this looks quite good.

.module may feel like a GNU as directive. I suggest .lto_module so that it is clear the directive is LTO internal stuff.

Alternatively, you can make .lto_discard reset the symbol list, then .lto_discard is sufficient and .module will not be needed.

llvm/lib/MC/MCParser/AsmParser.cpp
5841

Add some comments about their usage in LTO.

MaskRay added inline comments.Mar 17 2021, 12:01 AM
llvm/lib/LTO/LTO.cpp
757

DenseSet<CachedHashStringRef>

std::set is inefficient. If you don't order guarantee, prefer an unordered container.

llvm/lib/MC/MCParser/AsmParser.cpp
1890

Needs a test/MC/ELF test that reassignment does not error.

llvm/lib/MC/MCParser/ELFAsmParser.cpp
185

Needs a test/MC/ELF test that reassignment does not error.

llvm/test/LTO/X86/weak-asm.ll
13 ↗(On Diff #331165)

Drop -o - if you use <

ychen updated this revision to Diff 331322.Mar 17 2021, 11:02 AM
  • Use SmallSet for NonPrevailingAsmSymbols since it allocates less than DenseSet
  • Rename .module to .lto_module
ychen marked 2 inline comments as done.Mar 17 2021, 11:08 AM
ychen added inline comments.
llvm/lib/LTO/LTO.cpp
757

Agreed that std::set is not the most appropriate one. I used the SmallSet to fit the use case better. DenseSet seems allocate more than needed for the usage.

llvm/lib/MC/MCParser/AsmParser.cpp
1890

Something like llvm/test/MC/AsmParser/reassign.s for this patch?

MaskRay added inline comments.Mar 17 2021, 11:14 AM
llvm/lib/MC/MCParser/AsmParser.cpp
1890

Yes.

What do you think of dropping .lto_module? One directive .lto_discard is enough.

Alternatively, you can make .lto_discard reset the symbol list, then .lto_discard is sufficient and .module will not be needed.

Thanks for the review. Yeah, if we keep the ".lto_discard" then ".module" is not needed. In that case, it has to be legal to use ".lto_discard" without trailing symbols since a marker is still needed to separate the inlineasm block for each module. Otherwise, asm symbols in an irrelevant module could be discarded incorrectly.

ychen added a comment.EditedMar 17 2021, 11:18 AM

I mean ".lto_module" reveals the marker function explicitly. I'm fine with only using ".lto_discard" though. I could add comments to explain the usage above parseDirectiveLTODiscard. Please let me know your preference.

ychen updated this revision to Diff 331457.Mar 17 2021, 9:02 PM
  • Add additional tests
  • Add the missing case for .global

I mean ".lto_module" reveals the marker function explicitly. I'm fine with only using ".lto_discard" though. I could add comments to explain the usage above parseDirectiveLTODiscard. Please let me know your preference.

I prefer only using .lto_discard. .lto_module adds unneeded code.

ychen updated this revision to Diff 331590.Mar 18 2021, 9:36 AM
  • Simplify code by not using .lto_module
ychen added a comment.Mar 18 2021, 9:36 AM

I mean ".lto_module" reveals the marker function explicitly. I'm fine with only using ".lto_discard" though. I could add comments to explain the usage above parseDirectiveLTODiscard. Please let me know your preference.

I prefer only using .lto_discard. .lto_module adds unneeded code.

Makes sense. Patch updated.

Mostly looks good.

llvm/lib/LTO/LTO.cpp
815
833

If the previous .lto_discard directive is not empty, you'll need to reset the list to empty, otherwise can this discard some prevailing symbols?

llvm/test/MC/ELF/lto-discard.s
10

There needs to be a test checking that .lto_discard without arguments works.
You can use --defsym to place multiple tests in one file if keeping them in one file can help readers.
See some existing --defsym tests for example.

ychen updated this revision to Diff 331620.Mar 18 2021, 10:39 AM
  • address comments
ychen added inline comments.Mar 18 2021, 10:42 AM
llvm/lib/LTO/LTO.cpp
833

NonPrevailingAsmSymbols is empty initially for each module and the module without inlineasm block is not considered. I think it should be fine?

MaskRay accepted this revision.Mar 18 2021, 10:51 AM
MaskRay added inline comments.
llvm/lib/LTO/LTO.cpp
757

Use 2 to be consistent with SmallSet<StringRef, 2> LTODiscardSymbols;

833

I am thinking whether this scenario may happen and drop the prevailing foo accidentally.

.lto_discard foo
non-prevailing foo
prevailing foo

Perhaps this is impossible.

836

Don't dicard a symbol if there is a live .symver for it. better to have a test.

llvm/test/MC/ELF/lto-discard.s
4

-filetype=asm does not have effects. You need a -filetype=obj test to show that the foo definition is ignored.

This revision is now accepted and ready to land.Mar 18 2021, 10:51 AM
ychen updated this revision to Diff 331626.Mar 18 2021, 11:06 AM
  • beef up inline-asm-lto-discard.ll test
ychen updated this revision to Diff 331646.Mar 18 2021, 11:49 AM
  • Add a test case for .symver
  • Rename test file properly
ychen marked 3 inline comments as done.Mar 18 2021, 11:50 AM
ychen added inline comments.
llvm/test/MC/ELF/lto-discard.s
4

I'm checking .lto_discard is no-op here..

inline-asm-lto-discard.ll is beefed up to test this.

MaskRay added a comment.EditedMar 18 2021, 12:15 PM
  • Add a test case for .symver
  • Rename test file properly

Oh I missed EnableLTODiscardSymbols, which is different in LTO and non-LTO. Isn't it a unneeded implementation detail? You can just let .lto_discard unconditionally prevent definitions.
Yes, the name is a bit misnomer here, but that does not matter if you document clearly that it is an implementation detail of inline asm handling in LTO so that users will not misuse it.

MaskRay requested changes to this revision.Mar 18 2021, 12:16 PM
This revision now requires changes to proceed.Mar 18 2021, 12:16 PM
ychen updated this revision to Diff 331665.Mar 18 2021, 1:01 PM
  • Drop unnecessary EnableLTODiscardSymbols to make the implementation simpler.
MaskRay accepted this revision.Mar 18 2021, 1:19 PM
MaskRay added inline comments.
llvm/lib/MC/MCParser/AsmParser.cpp
5843

The LTO library emits this directive to discard non-prevailing symbols. We ignore symbol assignments and attribute changes for the specified symbols.

5850

You can add a --defsym=ERR=1 RUN line to the test/MC/ELF test to check the diagnostic message and position. See relocation-alias.s and `reloc-directive.s for examples.

llvm/test/LTO/X86/inline-asm-lto-discard2.ll
6

Nit: -filetype=asm is the default. Delete

This revision is now accepted and ready to land.Mar 18 2021, 1:19 PM

PS: ".lto_discard" is not strictly required but it would make the MC change much simpler. I didn't add any standalone MC tests because the only client for these two directives should be LTO.

Please fix the description.

ychen edited the summary of this revision. (Show Details)Mar 18 2021, 2:31 PM
ychen updated this revision to Diff 331697.Mar 18 2021, 3:03 PM
  • Add additional test cases based on comments.
ychen marked 3 inline comments as done.Mar 18 2021, 3:04 PM
MaskRay accepted this revision.Mar 18 2021, 3:21 PM
This revision was landed with ongoing or failed builds.Mar 18 2021, 3:34 PM
This revision was automatically updated to reflect the committed changes.
ychen added a comment.Mar 18 2021, 3:36 PM

@MaskRay Thanks a lot for the feedbacks.