This is an archive of the discontinued LLVM Phabricator instance.

[LTO] Suppress emission of empty combined module by default
ClosedPublic

Authored by khchen on Apr 28 2020, 2:44 AM.

Details

Summary

That unless the user requested an output object (--lto-obj-path), the an unused empty combined module is not emitted.

This changed is helpful for some target (ex. RISCV-V) which encoded the ABI info in IR module flags (target-abi). Empty unused module has no ABI info so the linker would get the linking error during merging incompatible ABIs. target-abi had discussed in http://lists.llvm.org/pipermail/llvm-dev/2020-January/138151.html.

Diff Detail

Event Timeline

khchen created this revision.Apr 28 2020, 2:44 AM
khchen planned changes to this revision.Apr 28 2020, 7:19 AM

sorry, my fault, there are two failing tests I need to fix first.

khchen updated this revision to Diff 260701.Apr 28 2020, 10:50 AM

fixed linker-script-symbols-assign.ll, now lto would not generate empty module (%t2.lto.o)
fixed typo and format.

I know you said you needed to fix a few test failures, but I have some comments/questions on the first version of the patch anyway.

llvm/include/llvm/LTO/Config.h
71

Since this only applies to the regular LTO object, would be better to call this something more specific, like AlwaysEmitRegularLTOObj.

llvm/lib/LTO/LTOBackend.cpp
503

Just to clarify my assumption, you are doing this here rather than down in codegen because for ThinLTO modules we want to produce an object even when an empty module is explicitly linked for some reason?

Rather than that series of checks, I think it would be better to just include a flag on the RegularLTOState to indicate whether addRegularLTO was ever called. It's clearer and probably less fragile.

Also - what is the value of calling the hook if it is an empty default module that wasn't added by the link, and that we aren't going to code gen?

llvm/tools/gold/gold-plugin.cpp
1057–1058

Why are all these changes needed, since you are addressing this within LTO to not emit the empty object file?

khchen marked 3 inline comments as done.Apr 28 2020, 9:44 PM

Hi @tejohnson
I'm very appreciate your help and comment,
because I'm not LTO expert, so if you have any good idea and concern in implementation
Please don't hesitate to correct me, thanks.

llvm/include/llvm/LTO/Config.h
71

okay, I will fix it.

llvm/lib/LTO/LTOBackend.cpp
503

Just to clarify my assumption, you are doing this here rather than down in codegen because for ThinLTO modules we want to produce an object even when an empty module is explicitly linked for some reason?

There are 3 tests got failed if moving this code into codegen,

  1. LLVM :: ThinLTO/X86/empty_module_with_cache.ll

https://github.com/llvm/llvm-project/blob/master/llvm/test/ThinLTO/X86/empty_module_with_cache.ll#L4
In current caching mechanism, it should work even if the module is empty.

  1. Clang :: CodeGen/thinlto_backend.ll

https://github.com/llvm/llvm-project/blob/master/clang/test/CodeGen/thinlto_backend.ll#L27
The output is empty file (not empty obj file) which llvm-nm can not recognize it.

  1. LLVM :: tools/gold/X86/cache.ll

https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/gold/X86/cache.ll#L27
after opt phase, %t2 (cache.ll.tmp2.o.4.opt.bc) is empty module, so there is no %t2 cache file.

Rather than that series of checks, I think it would be better to just include a flag on the RegularLTOState to indicate whether addRegularLTO was ever called. It's clearer and probably less fragile.

okay, I will try it.

Also - what is the value of calling the hook if it is an empty default module that wasn't added by the link, and that we aren't going to code gen?

Sorry, I don't understand the question, HookOnly is used to suppress codegen but still emit temp file for debugging, than the gold ld wasn't store empty Buf[Task] as file and link it.

llvm/tools/gold/gold-plugin.cpp
1057–1058

original implementation always generates file including empty module which generate empty file. (not object file)

This implementation reference lld.
In https://reviews.llvm.org/D78035#inline-719821 as you mentioned we can "possibly make the Files array a tuple with another bool flag indicating whether the obj file buffer was empty." but I could not find a good place to store empty info when calling lto::backend.

any suggestion?

tejohnson added inline comments.Apr 28 2020, 11:17 PM
llvm/lib/LTO/LTOBackend.cpp
503

Sorry, I don't understand the question, HookOnly is used to suppress codegen but still emit temp file for debugging, than the gold ld wasn't store empty Buf[Task] as file and link it.

What I meant was that I don't think we need to execute the hook, if this is an empty combined module that doesn't correspond to any incoming linked module and that we aren't going to codegen. I.e. don't even call codegen in this case.

llvm/tools/gold/gold-plugin.cpp
1057–1058

That was an alternate suggestion, instead of changing LTO, since you mentioned lld worked fine. Since you are suppressing the object file in LTO, is any change needed here? AddStream isn't called until down in codegen(), and with your change that isn't being executed. So presumably no change is needed here to prevent the empty object from being passed back to gold.

khchen updated this revision to Diff 260881.Apr 29 2020, 4:24 AM
khchen marked an inline comment as done.

address @tejohnson's comments

khchen marked 6 inline comments as done.Apr 29 2020, 4:28 AM
khchen added inline comments.
llvm/tools/gold/gold-plugin.cpp
1057–1058

You are right, thanks!

Thanks, the changes make more sense to me now with the gold plugin changes removed. I have some more comments below. Once this is cleaned up a bit more I will give it a quick test or so internally to make sure it doesn't break our flow (I don't think so but worth a check).

lld/test/ELF/lto/linker-script-symbols-assign.ll
6

Is this change still valid with your new approach of keeping a flag on the RegularLTO state object? Or is lld not even sending the object to LTO? If so, probably should check now that this file isn't created.

8

Looks like these orphan checks were left behind when a recent change updated the test...

llvm/include/llvm/LTO/Config.h
69

Suggest making the first sentence "Always emit a Regular LTO object even when it is empty because no Regular LTO modules were linked."

Also, s/usedful/useful/

llvm/lib/LTO/LTO.cpp
1033 ↗(On Diff #260881)

Actually, do we even need to call backend() if (!EmptyCombinedModule || C.AlwaysEmitRegularLTOObj) ? It doesn't seem like there should be any need to create a TM or call opt in that case either. And as I note in backend(), there's no need to do the splitCodeGen handling either.

llvm/lib/LTO/LTOBackend.cpp
499–504

Move this check out a level and guard both this and the splitCodeGen call the same way (split codegen isn't commonly used, but it makes sense to be consistent - splitCodeGen will ultimately call codegen()).

llvm/test/ThinLTO/X86/empty-module.ll
6

Presumably %t2.0 is the empty combined module. Should it be emitted after your change? Oh I see, this is related to your llvm-lto2 change. I'll add a comment there.

llvm/test/ThinLTO/X86/empty-object.ll
2

"empty combined module"

However, I'm unsure what this new test is trying to check that is different than the existing empty-module.ll.

Are you trying to confirm that we emit an empty ThinLTO module (corresponding to %t2.bc) but not the empty combined module? If so, please clarify in description.

3

rm -f %t.*
probably better

10

rm -f %t3.*
?

15

Is there a missing FileCheck corresponding to these?

llvm/tools/llvm-lto2/llvm-lto2.cpp
289

Why this change? This is inconsistent with the linker behavior under the corresponding option (e.g. gold plugin's thinlto-index-only).

khchen updated this revision to Diff 260946.Apr 29 2020, 9:45 AM
khchen marked an inline comment as done.

rebase master

khchen updated this revision to Diff 261121.Apr 29 2020, 9:21 PM
khchen marked an inline comment as done.
  1. Add empty combined module checking before call backend()
  2. Appling new approach, some testcasess need to updated and deleted. In checking module empty approach, it can suppress emitting optimzed empty combined module, but in new approach, it still emit combined module. I thought new approach behavior is acceptable. This case happend in linker-script-symbols-assign.ll.
  3. Delete llvm/test/ThinLTO/X86/empty-object.ll. I misunderstood -thinlto-distributed-indexes option meaning. I add this case just becasue becuase I modified llvm-lto2. I think testing in empty-module.ll is enough. This patch focus on empty combined module, I think we don't need to comfirm "empty ThinLTO module" still be emitted.
khchen marked 6 inline comments as done.Apr 29 2020, 9:25 PM
khchen added inline comments.
lld/test/ELF/lto/linker-script-symbols-assign.ll
6

In new approach this test passed because input bitcode is not empty, (it will be empty after opt phase).

llvm/lib/LTO/LTO.cpp
1033 ↗(On Diff #260881)

Yes, you are right, we don't need to call backend()

MaskRay added inline comments.Apr 29 2020, 9:51 PM
lld/test/ELF/lto/linker-script-symbols-assign.ll
6

Use ;; for comments. For newer tests we are enforcing this rule.

lld/test/ELF/lto/thinlto-obj-path.ll
18

%T is currently deprecated. You can use rm -fr %t.dir && mkdir %t.dir/objpath

20

--save-temps

Unless the single dash option is prevailing (-pie -shared etc), we prefer double-dash long options.

MaskRay added inline comments.Apr 29 2020, 10:13 PM
lld/test/ELF/lto/thinlto-obj-path.ll
18

rm -fr %t.dir && mkdir -p %t.dir/objpath

khchen updated this revision to Diff 261134.Apr 29 2020, 11:46 PM
khchen marked 2 inline comments as done.
  1. Rebase on master branch.
  2. update lld testcases
khchen marked 4 inline comments as done.Apr 29 2020, 11:49 PM

@MaskRay Thanks for your suggestion and reminder.

This looks good to me but let me give it some quick sanity checks internally today and will get back.

tejohnson accepted this revision.Apr 30 2020, 1:20 PM

lgtm

Before you submit, please update the patch description to reflect the current behavior (e.g. that unless the user requested an output object, the an unused empty combined module is not emitted). Also the patch title could be more specific, e.g. [LTO] Suppress emission of empty combined module by default

This revision is now accepted and ready to land.Apr 30 2020, 1:20 PM
khchen retitled this revision from [LTO] Suppress emission of the empty object file to [LTO] Suppress emission of empty combined module by default.May 2 2020, 7:27 AM
khchen edited the summary of this revision. (Show Details)

That unless the user requested an output object (-lto-obj-path)

Use --lto-obj-path. -l is a very common and conflicting option.

lld/test/COFF/lto-obj-path.ll
14

Use either lld or lld-link

khchen updated this revision to Diff 261704.May 3 2020, 10:29 AM

address @MaskRay's review feedback

khchen edited the summary of this revision. (Show Details)May 3 2020, 10:29 AM
MaskRay accepted this revision.May 3 2020, 10:58 AM
MaskRay added inline comments.
lld/test/ELF/lto/linker-script-symbols-assign.ll
7

s/ld/lld/

khchen updated this revision to Diff 261734.May 3 2020, 8:17 PM

s/ld/lld/

This revision was automatically updated to reflect the committed changes.