[clang] A Unified LTO Bitcode Frontend

[ormris]

Summary

The unified LTO pipeline creates a single LTO bitcode structure that can be
used by Thin or Full LTO. This means that LKTO mode can be chosen at link
time and that all LTO bitcode produced by the pipeline is compatible, from
an optimization perspective. This makes the behavior of LTO a bit more
predictable by normalizing the set of LTO features supported by each LTO
bitcode file.

Example usage:

clang -flto -funified-lto foo.c
clang -flto -funified-lto bar.c

ld.lld --lto=full foo.o bar.o -o baz.elf

The RFC discussing the details and rational for this change is here:
https://discourse.llvm.org/t/rfc-a-unified-lto-bitcode-frontend/61774

[ormris]

Add LLD driver changes to this patch. Disable split LTO units by default. As discussed here: https://discourse.llvm.org/t/rfc-a-unified-lto-bitcode-frontend/61774/15

[ormris]

Changelog:

• Rebased

[ormris]

Remove duplicated LLVM tests.

[ormris]

Attempt to fix premerge checks

[ormris]

clang-format

[ormris]

Use the ThinLTO pre-link pipeline as the Unified LTO pre-link pipeline, as discussed in the RFC.

[ormris]

Rebase

[ormris]

gentle ping

[MaskRay]

The lld side has no test. I think we normally create separate patches for clang and lld. So you may want to reuse D123805 for lld changes.
See lld/test/ELF/lto/ for tests.

[MaskRay]

clang/lib/CodeGen/BackendUtil.cpp

1008

Simplify this with if (IsThinLTO || (IsLTO && CodeGenOpts.UnifiedLTO))

clang/lib/Driver/ToolChains/Clang.cpp

4772

Search for Args.addOptInFlag

4919

Do you want to use isPS() special case both PS4 and PS5? Or is it that just PS4 is special?

Is only PS4 is special, the summary should mention why.

7306–7309

Add a comment why PS4 is special.

clang/test/CodeGen/asan-unified-lto.ll

9

Consider checking a few more things including the module flag metadata beside the global variable name.

16

ptr. Avoid obsoleted typed pointers.

clang/test/CodeGen/emit-summary-index.c

17

Add a comment with /// (for non-RUN non-CHECK lines) about the purpose of the two RUN lines.

clang/test/CodeGen/unified-lto-pipeline.c

2

Without --target, this uses the default target triple which may fail for some exotic platforms.

Prefer %clang_cc1 for non-driver tests. Instead of comparing the output files, test -fdebug-pass-manager output.

[MaskRay]

clang/test/Driver/unified-whole-program-vtables.c

1 ↗

(On Diff #529091)

I think it'll be more readable if you place all -funitied-lto related driver tests in one file, instead of spreading them into unified-whole-program-vtables.c and lto-unit.c

Use --target= for new tests. -target has been deprecated since Clang 3.4

[ormris]

clang/lib/CodeGen/BackendUtil.cpp

1008

Fixed

1052–1054

That's a good point. I've removed this change.

clang/lib/Driver/ToolChains/Clang.cpp

4772

There's driver code that depend on the value of this flag, so I do need to store it. Args.addOptInFlag doesn't return the value of the flag. Is there an advantage to calling addOptInFlag, then hasFlag?

4919

Fixed.

7306–7309

Fixed.

7329

Fixed

clang/test/CodeGen/asan-unified-lto.ll

16

Fixed

clang/test/CodeGen/emit-summary-index.c

17

Fixed

clang/test/CodeGen/unified-lto-pipeline.c

2

Why test the pipeline rather than binary equivalence? Identical bitcode files is a key part of this feature.

clang/test/Driver/unified-whole-program-vtables.c

1 ↗

(On Diff #529091)

Agreed. Fixed.

[ormris]

@MaskRay There are very few LLD changes in this review, but we can split this out if it would be more convenient.

[MaskRay]

In D123804#4486130, @ormris wrote:

@MaskRay There are very few LLD changes in this review, but we can split this out if it would be more convenient.

Yeah, preferably split. I use this convention for most features.

[ormris]

Remove LLD changes from this patch. They're now in D123805.

[MaskRay]

Thank you for the update.

IIUC -flto=full -funified-lto, -flto -funified-lto, and -flto=thin -funified-lto are identical.
-flto=full -funified-lto may be confusing as we treat it as it -flto=thin and use buildThinLTOPreLinkDefaultPipeline.

In the absence of -flto, -funified-lto seems a no-op (I do not see a test).

I wonder whether we should change -funified-lto to become -flto=unified instead.
(For the lld option, I guess --lto-mode={thin,full} may be clearer than --lto={thin,full}, but I don't have a strong opinion... )

clang/lib/Frontend/CompilerInvocation.cpp

1779

hasArg if you don't use A

clang/test/Driver/lto-unit.c

9 ↗

(On Diff #538803)

Is NOUNIT unused?

clang/test/Driver/unified-lto.c

6

Is NOUNIT-NOT used?

10

This test may be better placed in whole-program-vtables.c where this diagnostic is tested.

[MaskRay]

[WIP][clang][lld] A Unified LTO Bitcode Frontend

You may change the titles of this patch and the lld one... I guess it is named WIP so I ignored reading it for a long time (sorry!).

[ormris]

IIUC -flto=full -funified-lto, -flto -funified-lto, and -flto=thin -funified-lto are identical.

They're not completely identical. They change the flag passed to the linker when the linker is invoked by the compiler. I think it's worth preserving that workflow. It's used fairly often for manually compiling a project (e.g. clang -flto=thin -funified-lto hello.cpp), but also by CMake.

In the absence of -flto, -funified-lto seems a no-op (I do not see a test).

Good point. Fixed.

For the lld option, I guess --lto-mode={thin,full}

Maybe.... I was trying to mirror -flto and reduce the amount of typing required.

clang/lib/Frontend/CompilerInvocation.cpp

1779

Fixed.

clang/test/Driver/lto-unit.c

9 ↗

(On Diff #538803)

Fixed.

clang/test/Driver/unified-lto.c

6

No, but it should be. Fixed.

10

Fixed

[ormris]

Address comments

[MaskRay]

In D123804#4490463, @ormris wrote:

IIUC -flto=full -funified-lto, -flto -funified-lto, and -flto=thin -funified-lto are identical.

They're not completely identical. They change the flag passed to the linker when the linker is invoked by the compiler. I think it's worth preserving that workflow. It's used fairly often for manually compiling a project (e.g. clang -flto=thin -funified-lto hello.cpp), but also by CMake.

OK, sounds good. I think the description can be updated to include some examples.
The discourse link has quite a lot of discussions and the commit message/summary can be a good place to summarize all the information.
If I were a newcomer, I would find the discourse link too daunting to comprehend...

In the absence of -flto, -funified-lto seems a no-op (I do not see a test).

Good point. Fixed.

For the lld option, I guess --lto-mode={thin,full}

Maybe.... I was trying to mirror -flto and reduce the amount of typing required.

clang/test/CodeGen/unified-lto-pipeline.c

2

cmp %t.0 %t.1 should be retained. My point is that we probably should test some output to demonstrate that his is a ThinLTO pipeline.

// RUN: not cmp %t.2 %t.3 is probably less useful if we can demonstrate that the pipeline is different.

[ormris]

clang/test/CodeGen/unified-lto-pipeline.c

2

Ah that makes sense. Fixed.

[ormris]

Address comments. I've updated the review description as well.

[MaskRay]

Sorry for extra nitpicking:)

[clang] A Unified LTO Bitcode Frontend

I think we typically start with a verb and don't capitalize every words, e.g. [clang] Support Unified LTO Bitcode frontend

This means that LKTO mode can be chosen at link [...]

typo: LKTO?

Feel free to incorporate the following to your summary:

# Compile and link. Select regular LTO at link time.
clang -flto -funified-lto -fuse-ld=lld foo.c

# Compile and link. Select ThinLTO at link time.
clang -flto=thin -funified-lto -fuse-ld=lld foo.c

# Link separately, using ThinLTO.
clang -c -flto -funified-lto foo.c  # -flto={full,thin} are identical in terms of compilation actions
clang -flto=thin -fuse-ld=lld foo.o  # pass --lto=thin to ld.lld

# Link separately, using regular LTO.
clang -c -flto -funified-lto foo.c
clang -flto -fuse-ld=lld foo.o  # pass --lto=full to ld.lld

clang/lib/Driver/ToolChains/Clang.cpp

4772

One blank line is sufficient.

[MaskRay]

clang/test/CodeGenCXX/unified-cfi-lto.cpp

4

Did you forget FileCheck?

clang/test/Driver/unified-lto.c

10

If the path of %s contains -funified-lto, the NOT pattern will fail spuriously. -funified-lto may be unique enough to not cause an issue in practice, but other options did cause an issue IIRC. The conventional solution is to use a double quote to make it really unique enough.
In addition, we typically start with "-cc1" to indicate that we are testing cc1 command line.

// NOUNILTO:     "-cc1"
// NOUNILTO-NOT: "-funified-lto

clang/test/Driver/whole-program-vtables.c

9

Add a comment /// -funified-lto does not imply -flto, so we still get an error that fwhole-program-vtables has no effect without -flto

[ormris]

Thanks! Pushed as 048a0c246908291c82d2f4531d3df45a4c4a8a18.

[pcc]

Looks like this caused some buildbot failures, e.g.

https://lab.llvm.org/buildbot/#/builders/168/builds/14547

Can you please take a look?

[ormris]

Yes, looking now.

[ormris]

@pcc I'll remove the failing test while I work on a reproducing this.