This is an archive of the discontinued LLVM Phabricator instance.

[clang] Preliminary fat-lto-object support
ClosedPublic

Authored by paulkirth on Mar 23 2023, 5:56 PM.

Details

Summary

Fat LTO objects contain both LTO compatible IR, as well as generated
object code. This allows users to defer the choice of whether to use LTO
or not to link-time. This is a feature available in GCC for some time,
and makes the existing -ffat-lto-objects flag functional in the same
way as GCC's.

This patch adds support for that flag in the driver, as well as setting the
necessary codegen options for the backend. Largely, this means we select
the newly added pass pipeline for generating fat objects.

Users are expected to pass -ffat-lto-objects to clang in addition to one
of the -flto variants. Without the -flto flag, -ffat-lto-objects has no
effect.

// Compile and link. Use the object code from the fat object w/o LTO.
clang -fno-lto -ffat-lto-objects -fuse-ld=lld foo.c

// Compile and link. Select full LTO at link time.
clang -flto -ffat-lto-objects -fuse-ld=lld foo.c

// Compile and link. Select ThinLTO at link time.
clang -flto=thin -ffat-lto-objects -fuse-ld=lld foo.c

// Compile and link. Use ThinLTO with the UnifiedLTO pipeline.
clang -flto=thin -ffat-lto-objects -funified-lto -fuse-ld=lld foo.c

// Compile and link. Use full LTO with the UnifiedLTO pipeline.
clang -flto -ffat-lto-objects -funified-lto -fuse-ld=lld foo.c

// Link separately, using ThinLTO.
clang -c -flto=thin -ffat-lto-objects foo.c
clang -flto=thin -fuse-ld=lld foo.o -ffat-lto-objects # pass --lto=thin --fat-lto-objects to ld.lld

// Link separately, using full LTO.
clang -c -flto -ffat-lto-objects foo.c
clang -flto -fuse-ld=lld foo.o # pass --lto=full --fat-lto-objects to ld.lld

Original RFC: https://discourse.llvm.org/t/rfc-ffat-lto-objects-support/63977

Depends on D146776

Diff Detail

Event Timeline

paulkirth created this revision.Mar 23 2023, 5:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 5:56 PM
paulkirth requested review of this revision.Mar 23 2023, 5:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 5:56 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
paulkirth updated this revision to Diff 508773.Mar 27 2023, 1:16 PM
paulkirth retitled this revision from [clang] Preliminary fat-lot-object support to [clang] Preliminary fat-lto-object support.

Fix typo in title

Add relese note.

paulkirth updated this revision to Diff 531534.Jun 14 2023, 3:16 PM

Rebase and pass fat-lto-objects to LLD from clang.

tejohnson added inline comments.Jun 15 2023, 6:55 AM
clang/lib/CodeGen/BackendUtil.cpp
1057

Can you expand the comment a bit - specifically why it won't end up in the above handling? I assume the Action type is different for FatLTO?

clang/lib/Driver/ToolChains/CommonArgs.cpp
622 ↗(On Diff #531534)

nit: missing spaces

625 ↗(On Diff #531534)

Needs a test

clang/test/CodeGen/embed-lto-fatlto.c
3

Can you also test -flto=thin, with and without LTO splitting enabled?

paulkirth added inline comments.Jun 15 2023, 3:24 PM
clang/lib/CodeGen/BackendUtil.cpp
1057

yes, we're not either of the above Action types. I'll expand that description to include more context, but maybe it should also be an else if(...)to make this code's relationship with the above more clear?

clang/lib/Driver/ToolChains/CommonArgs.cpp
622 ↗(On Diff #531534)

TY. Looks like it needs a git clang-format. I'll be sure to run that again when I address the other comments.

625 ↗(On Diff #531534)

Noted.

clang/test/CodeGen/embed-lto-fatlto.c
3

will do.

paulkirth marked an inline comment as done.

Rebase, address comments, and add missing test cases.

tejohnson accepted this revision.Jun 16 2023, 12:52 PM

lgtm

clang/test/Driver/fatlto.c
1 ↗(On Diff #532212)

Is this empty file meant to contain something?

This revision is now accepted and ready to land.Jun 16 2023, 12:52 PM
paulkirth updated this revision to Diff 532272.Jun 16 2023, 1:04 PM

Rebase and fix run line in test

paulkirth marked an inline comment as done.Jun 16 2023, 1:58 PM
paulkirth added inline comments.
clang/test/Driver/fatlto.c
1 ↗(On Diff #532212)

No, this was just a mistake in my commit. I had the linker checks in another empty file, following the example from some of the lld/gold plugin tests and realized those should have just been in the existing file. I forgot to remove the empty file from git when updating the patch. I've removed it now.

paulkirth marked 4 inline comments as done.Jun 16 2023, 1:59 PM
MaskRay accepted this revision.Jun 17 2023, 5:05 PM

There is a -fno-fat-lto-objects issue, but otherwise looks good after some nits are addressed. Thanks!

clang/docs/ReleaseNotes.rst
250 ↗(On Diff #532272)
clang/include/clang/Driver/Options.td
2268

We just need the pos flag for CC1Option, so use OptInCC1FFlag

clang/lib/Driver/ToolChains/Clang.cpp
7265

This does not consider the negative option.

if (IsUsingLTO) {
  if (Arg *A = Args.getLastArg(options::OPT_ffat_lto_objects, options::OPT_fno_fat_lto_objects)) {
    if (A->getOption().matches(options::OPT_ffat_lto_objects)) {
      CmdArgs.push_back(...)
      A->render(Args, CmdArgs);
    }
  }
}

or move getLastArg outside of if (IsUsingLTO) to avoid -Wunused-command-line-argument in the absence of -flto=.

This code block probably should be moved before fglobal_isel to be closer to other LTO code.

clang/test/CodeGen/embed-lto-fatlto.c
2

Remove -S. In CC1, -emit-llvm wins.

9
12
clang/test/Driver/fatlto-objects.c
2

Delete the REQUIRES. The driver code should be portable as long as we only use -###.

2

fat-lto-objects.c or ffat-lto-objects.c

3

Delete -fintegrated-as. It's the default.

5

Use -SAME: whenever applicable.

8

You may test that -ffat-lto-objects in the absence of -flto= gives a warning: argument unused during compilation: '-ffat-lto-objects' warning

19
MaskRay added inline comments.Jun 17 2023, 5:06 PM
clang/docs/ReleaseNotes.rst
250 ↗(On Diff #532272)

bitcode => LLVM bitcode

MaskRay added inline comments.Jun 17 2023, 5:36 PM
clang/lib/Driver/ToolChains/Clang.cpp
7266

We also need a err_drv_unsupported_opt_for_target error for non-ELF

paulkirth updated this revision to Diff 533433.Jun 21 2023, 4:43 PM
paulkirth marked 12 inline comments as done.

Rebase and address comments

  • Update & rename tests
  • Fix typos
  • Handle options
clang/include/clang/Driver/Options.td
2268

Wouldn't that prevent me from setting CodeGenOpts<"FatLTO">? AFAICT OptInCC1Flag doesn't take a CodeGenOpts since it isn't an OptionsFlag.

clang/test/CodeGen/embed-lto-fatlto.c
9

Extremely useful to know. Not sure how I've missed that until now. Thanks!

clang/test/Driver/fatlto-objects.c
8

Since I've slightly refactored the driver code based on your comments, we shouldn't get that warning anymore. I've tested that instead, but we can also drop it, since I'm not sure how much value that adds.

As mentioned, you may consider landing llvm patch then wait a bit so that (a) people can experiment with the clang patch better (b) prevent the llvm/clang patches to be both reverted, if some issue has been identified with the llvm patch.
(Don't worry that a feature doesn't have contiguous commits. It's common.)

I do not accept the lld part now, just to have more time to ensure the llvm and clang part is mature :)

As mentioned, you may consider landing llvm patch then wait a bit so that (a) people can experiment with the clang patch better (b) prevent the llvm/clang patches to be both reverted, if some issue has been identified with the llvm patch.

Well, any later than Friday morning and it will have to wait until I get back from vacation. :) That isn't a big deal, though. and yeah, given the llvm patch had to be reverted twice, I'm fine w/ being a bit cautious.

(Don't worry that a feature doesn't have contiguous commits. It's common.)

Yeah, I'm just trying to keep this stack up to date w/ the rebases. When I've let them get very much out of sync it's caused my headaches in the past w/ phabricator not being able to apply the patch or because I hadn't kept them up to date and downloaded the patch from phabricator. I really miss Gerrits ability to upload the whole stack at once and have all the correct relationships. IDK if things will be better w/ github PRs, but I guess we'll all find out soon.

I do not accept the lld part now, just to have more time to ensure the llvm and clang part is mature :)

No problem. There's still some non trivial test improvements to implement on those anyway. Plus I need to figure out how to handle the archive case, which I don't think will be very hard, but I haven't looked at that code yet either.

As mentioned, you may consider landing llvm patch then wait a bit so that (a) people can experiment with the clang patch better (b) prevent the llvm/clang patches to be both reverted, if some issue has been identified with the llvm patch.

Well, any later than Friday morning and it will have to wait until I get back from vacation. :) That isn't a big deal, though. and yeah, given the llvm patch had to be reverted twice, I'm fine w/ being a bit cautious.

Sounds good!

(Don't worry that a feature doesn't have contiguous commits. It's common.)

Yeah, I'm just trying to keep this stack up to date w/ the rebases. When I've let them get very much out of sync it's caused my headaches in the past w/ phabricator not being able to apply the patch or because I hadn't kept them up to date and downloaded the patch from phabricator. I really miss Gerrits ability to upload the whole stack at once and have all the correct relationships. IDK if things will be better w/ github PRs, but I guess we'll all find out soon.

I hope that this will be useful: git rebase -i origin/main -x 'arc diff "HEAD^"'

I do not accept the lld part now, just to have more time to ensure the llvm and clang part is mature :)

No problem. There's still some non trivial test improvements to implement on those anyway. Plus I need to figure out how to handle the archive case, which I don't think will be very hard, but I haven't looked at that code yet either.

Thanks. I forgot that I mentioned archives:(

paulkirth updated this revision to Diff 539346.Jul 11 2023, 5:10 PM

Rebase and try to accomodate Unified LTO changes.

Based on the Unified LTO patches, I think this is the correct handling for
FatLTO, but I'd like to get a second opinion before landing this.

CC: @ormris

MaskRay added inline comments.Jul 11 2023, 5:14 PM
clang/lib/Driver/ToolChains/CommonArgs.cpp
623 ↗(On Diff #539346)

New lld long options only allow -- form. So use two dashes.

paulkirth updated this revision to Diff 539584.Jul 12 2023, 8:38 AM

Use -- for LLD options

Rebase and try to accomodate Unified LTO changes.

Based on the Unified LTO patches, I think this is the correct handling for
FatLTO, but I'd like to get a second opinion before landing this.

CC: @ormris

Would you mind testing the behavior when both -funified-lto -ffat-lto-objects are specified and mentioning it in the summary? Some concrete instructions for end users will be very useful. See https://reviews.llvm.org/D123804#4491107 for what I suggested for -funified-lto.

MaskRay added inline comments.Jul 12 2023, 9:48 PM
clang/docs/ReleaseNotes.rst
268 ↗(On Diff #539584)

You can insert the https://reviews.llvm.org/D146777 link here. It seems that libc++ and lld use a link more frequently.

paulkirth updated this revision to Diff 540192.Jul 13 2023, 3:06 PM
paulkirth marked an inline comment as done.
paulkirth edited the summary of this revision. (Show Details)

Update summary w/ instructions for using -ffat-lto-objects

Thanks for the update. Can you add a comment for the -funified-lto combination? It's unclear what it does...

clang -flto=thin -ffat-lto-objects -funified-lto -fuse-ld=lld foo.c

I've left some comments about missing test coverage.

clang/lib/Driver/Driver.cpp
4645

This part is not tested.

clang/lib/Driver/ToolChains/CommonArgs.cpp
621 ↗(On Diff #540192)

Tell LLD to find and use .llvm.lto section in regular relocatable object files.

clang/test/CodeGen/embed-fat-lto-objects.c
1 ↗(On Diff #540192)

embed- in the filename seems unneeded? Perhaps just fat-lto-objects.c

3 ↗(On Diff #540192)

We need a -emit-obj test with // REQUIRES: x86-registered-target.
Use llvm-readelf to check that .llvm.lto section is present.

We also need a -S test, otherwise the behavior of driver clang -S -ffat-lto-objects is untested.

clang/test/Driver/fat-lto-objects.c
10 ↗(On Diff #540192)

This NOT pattern has no effect as warnings are usually emitted before -cc1.

You can use --implicit-check-not=warning:

Thanks for the update. Can you add a comment for the -funified-lto combination? It's unclear what it does...

ugh, I had put comments above each compiler invocation, but forgot that # makes lines ignored in the commit message. Thanks for pointing that out, as I hadn't noticed after updating.

clang -flto=thin -ffat-lto-objects -funified-lto -fuse-ld=lld foo.c

I've left some comments about missing test coverage.

ah, sorry, for some reason I read your last comment as test locally to confirm it works instead of the obvious write a proper test. I will correct that shortly.

clang/test/CodeGen/embed-fat-lto-objects.c
3 ↗(On Diff #540192)

Sure, I can do both of those, but as a general question, isn't the generation of assembly the responsibility of the backend? I believe we have some tests that use llc on the the modules that have gone through the FatLTO pipeline. Is that insufficient?

Also, as a best practice, do you know if there are specific types of changes to clang that imply that we'd need to test -S? Sorry for all the questions, but I'm trying to get a bit better in how I structure my patches and approach testing.

clang/test/Driver/fat-lto-objects.c
10 ↗(On Diff #540192)

I'm happy to try that suggestion, but isn't this testing the generated cc1 command from the driver(e.g., because we're using %clang and not %clang_cc1)? I think that should still produce the warning, shouldn't it?

It's been a while since I made the patch, but I recall writing this test, having it fail, and then applying the change to stop it from being an unused argument...

Also, thanks for all the great feedback on testing. Despite writing many(well, more than a few at least) lit tests, I'm still surprised by lots of behaviors, and your suggestions have been very helpful.

paulkirth added inline comments.Jul 14 2023, 9:25 AM
clang/lib/Driver/Driver.cpp
4645

oh, feel free to disregard my Q about -S. I missed this comment, and why you've asked for that is now obvious.

MaskRay added inline comments.Jul 14 2023, 9:39 AM
clang/test/CodeGen/embed-fat-lto-objects.c
3 ↗(On Diff #540192)

Ah, sorry for my inaccurate suggestion. You are right.

To test clang/lib/Driver/Driver.cpp, we need to test the cc1 options of these driver options:

  • -S -ffat-lto-objects -flto
  • -S -emit-llvm -ffat-lto-objects -flto
  • -c -ffat-lto-objects -flto (already tested)

test/CodeGen doesn't need more tests.

clang/test/Driver/fat-lto-objects.c
10 ↗(On Diff #540192)

Yes, test/Driver just tests how Clang Driver generates warnings/errors and cc1 options. If there is a warning, it will be before the cc1 line, so // CHECK-CC-NOLTO-NOT: warning: argument unused during compilation: '-ffat-lto-objects' after cc1 will have no effect.

% myclang -flto -ffat-lto-objects -pie -c a.cc '-###'
clang version 17.0.0
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /tmp/Debug/bin
clang: warning: argument unused during compilation: '-pie' [-Wunused-command-line-argument]
 (in-process)
 "/tmp/Debug/bin/clang-17" "-cc1" ...

Actually, we usually don't want to check that a certain warning doesn't occur. Rather, we want to test no warning occurs, as it's easy to adjust a warning message and get stale tests.

In this case, I think I find a better test strategy: -fdriver-only -v -Werror. -fdriver-only ensures that an error changes the exit code and lit does check the exit code, fulfilling our testing requirement.

% myclang -fdriver-only -v -Werror -flto -ffat-lto-objects -pie -fdriver-only -c a.cc
clang: error: argument unused during compilation: '-pie' [-Werror,-Wunused-command-line-argument]
% echo $?
1

Also, thanks for all the great feedback on testing. Despite writing many(well, more than a few at least) lit tests, I'm still surprised by lots of behaviors, and your suggestions have been very helpful.

NP! Glad to help. I have spent too much time fiddling with driver options...

paulkirth updated this revision to Diff 540559.Jul 14 2023, 1:57 PM
paulkirth marked an inline comment as done.
paulkirth edited the summary of this revision. (Show Details)

Revise tests + update summary

  • Add driver tests for -S
  • Add codegen tests for -S
  • Test Unified LTO codegen
  • Use -fdriver-only to test that -ffat-lto-objects isn't unused
  • Test that the embedded bitcode has the correct module options set in object files
  • Check that the .llvm.lto section exists when we generate ELF
  • Make sure we set any FatLTO related module flags, even for other BackendActions(e.g., -emit-llvm), so that the IR emitted will match the IR embedded in the object file

I realized when writing the UnifiedLTO test that the existing implementation
wouldn't actually emit the correct IR using -emit-llvm for FatLTO. The flags
were set correctly in the .llvm.lto section, so I slightly adjusted the way we
set flags for the FatLTO case, so that -ffat-lto-objects -emit-llvm will have
the same module flags. The issue was that FatLTO isn't mutually exclusive w/
the other BackendActions, so my use of if else wasn't correct, and stopped us
from adding any flags when we used -emit-llvm. I missed this, since we set the
flags the same way.

The new tests in CodeGen may be a bit overkill, but they were already helpful in
finding this subtle bug.

MaskRay accepted this revision.Jul 15 2023, 2:17 PM

Thank you for the update. I think this Clang patch is good enough to land even if the lld part is still in review.

This revision was automatically updated to reflect the committed changes.

Small forward fix for the documentation in D155490. If something else crops up I can revert both and reland.