This is an archive of the discontinued LLVM Phabricator instance.

[IRSymTab] Set FB_used on llvm.compiler.used symbols
ClosedPublic

Authored by MaskRay on Mar 2 2021, 12:39 AM.

Details

Summary

IR symbol table does not parse inline asm. A symbol only referenced by inline
asm is not in the IR symbol table, so LTO does not know that the definition (in
another translation unit) is referenced and may internalize it, even if that
definition has __attribute__((used)) (which lowers to llvm.compiler.used on
ELF targets since D97446).

// cabac.c
__attribute__((used)) const uint8_t ff_h264_cabac_tables[...] = {...};

// h264_cabac.c
  asm("lea ff_h264_cabac_tables(%rip), %0" : ...);

__attribute__((used)) is the recommended way to tell the compiler there may
be inline asm references, so the usage is perfectly fine. This patch
conservatively sets the FB_used bit on llvm.compiler.used symbols to work
around the IR symbol table limitation. Note: before D97446, Clang never emitted
symbols in the llvm.compiler.used list, so this change does not punish any
Clang emitted global object.

Without the patch, ff_h264_cabac_tables may be assigned to a non-external
partition and get internalized. Then we will get a linker error because the
cabac.c definition is not exposed.

Diff Detail

Event Timeline

MaskRay created this revision.Mar 2 2021, 12:39 AM
MaskRay requested review of this revision.Mar 2 2021, 12:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2021, 12:39 AM

Are we missing a test that goes all the way from attribute((used)) -> IR -> LTO? It seems such a test would have helped catch the issue caused by the change of that attribute.

llvm/test/ThinLTO/X86/asm.ll
6

Should we be checking the output somewhere? I'm assuming there is a missing FileCheck that corresponds to the CHECK further below.

11

What is the behavior here without this patch?

MaskRay updated this revision to Diff 327870.Mar 3 2021, 11:43 AM
MaskRay marked an inline comment as done.

Add missing llvm-dis | FileCheck

This issue is like a pretty corner case thing. We do not parse inline asm instructions, so we don't know its IR symbol table.
This combined with the fact that (we previously used llvm.used) made it not a problem.

Are we missing a test that goes all the way from attribute((used)) -> IR -> LTO? It seems such a test would have helped catch the issue caused by the change of that attribute.

To me this proposal feels like a bit closer to cross-project testing: https://lists.llvm.org/pipermail/llvm-dev/2021-January/148121.html
Such tests are useful but I haven't figured out how to do it for LTO.
(I can see some clang/test/CodeGen/thinlto* tests but I am unsure whether a attribute((used))->llvm-lto test should be placed there.)

The majority tests are layered, e.g. touching LLVM IR transformation should generally not affect clang tests; touching LLVM backend should not affect LLVM IR transformation tests.

llvm/test/ThinLTO/X86/asm.ll
6

Forgot it. Sorry..

11

This is not changed. ff_h264_cabac_tables is not in the IR symbol table.

This issue is like a pretty corner case thing. We do not parse inline asm instructions, so we don't know its IR symbol table.
This combined with the fact that (we previously used llvm.used) made it not a problem.

Are we missing a test that goes all the way from attribute((used)) -> IR -> LTO? It seems such a test would have helped catch the issue caused by the change of that attribute.

To me this proposal feels like a bit closer to cross-project testing: https://lists.llvm.org/pipermail/llvm-dev/2021-January/148121.html
Such tests are useful but I haven't figured out how to do it for LTO.
(I can see some clang/test/CodeGen/thinlto* tests but I am unsure whether a attribute((used))->llvm-lto test should be placed there.)

There's already a bunch of tests in that directory that use llvm-lto or llvm-lto2 in some fashion. I think it would be good to add this test, since it's really the only way to ensure that the attribute is having the intended effect. It's often added specifically for the LTO case to avoid symbol deletion.

The majority tests are layered, e.g. touching LLVM IR transformation should generally not affect clang tests; touching LLVM backend should not affect LLVM IR transformation tests.

MaskRay updated this revision to Diff 327909.Mar 3 2021, 1:34 PM

Add a clang/test/CodeGen/ test

Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2021, 1:34 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
tejohnson added inline comments.Mar 3 2021, 2:54 PM
clang/test/CodeGen/thinlto-inline-asm2.c
10 ↗(On Diff #327909)

Is this other file needed for the test?

llvm/test/ThinLTO/X86/asm.ll
11

Same question here - is this file needed to test the handling of the llvm.compiler.used in the other file?

MaskRay added inline comments.Mar 3 2021, 3:28 PM
clang/test/CodeGen/thinlto-inline-asm2.c
10 ↗(On Diff #327909)

This is test the limitation.

If we fix the tracking for inline asm references (this traverses instructions and I don't know whether it should be fixed):

without the reference, the ff_h264_cabac_tables will be internalized if we don't have a reference. The test ensures that such improvement can be detected.

llvm.compiler.used means the references are seen by the linker, and do not need to suppress internalization if the linker has full knowledge.

llvm/test/ThinLTO/X86/asm.ll
11

See my answer above.

tejohnson accepted this revision.Mar 3 2021, 4:08 PM

lgtm with change suggested below. Thanks!

clang/test/CodeGen/thinlto-inline-asm2.c
10 ↗(On Diff #327909)

Ok, that makes sense. But then please add the same llvm-nm check with comment here as in the other test, so that it fails if that gets changed - I don't think as is this test will fail if that improvement is made.

This revision is now accepted and ready to land.Mar 3 2021, 4:08 PM
MaskRay updated this revision to Diff 327958.Mar 3 2021, 4:18 PM
MaskRay edited the summary of this revision. (Show Details)

Add llvm-nm checks

MaskRay marked 4 inline comments as done.Mar 3 2021, 4:18 PM
This revision was landed with ongoing or failed builds.Mar 3 2021, 4:22 PM
This revision was automatically updated to reflect the committed changes.