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

[MaskRay]

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.

[tejohnson]

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

7

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

12

What is the behavior here without this patch?

[MaskRay]

Add missing llvm-dis | FileCheck

[MaskRay]

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.

In D97755#2600903, @tejohnson wrote:

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

7

Forgot it. Sorry..

12

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

[tejohnson]

In D97755#2601054, @MaskRay wrote:

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.

In D97755#2600903, @tejohnson wrote:

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]

Add a clang/test/CodeGen/ test

[tejohnson]

clang/test/CodeGen/thinlto-inline-asm2.c

11

Is this other file needed for the test?

llvm/test/ThinLTO/X86/asm.ll

12

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

[MaskRay]

clang/test/CodeGen/thinlto-inline-asm2.c

11

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

12

See my answer above.

[tejohnson]

lgtm with change suggested below. Thanks!

clang/test/CodeGen/thinlto-inline-asm2.c

11

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.

[MaskRay]

Add llvm-nm checks

clang/test/CodeGen/thinlto-inline-asm2.c

+// REQUIRES: x86-registered-target
+
+// RUN: split-file %s %t
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -flto=thin -emit-llvm-bc %t/a.c -o %t/a.bc
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -flto=thin -emit-llvm-bc %t/b.c -o %t/b.bc
+// RUN: llvm-nm %t/a.bc | FileCheck %s --check-prefix=NM
+
+// RUN: llvm-lto2 run %t/a.bc %t/b.bc -o %t/out -save-temps -r=%t/a.bc,ref,plx -r=%t/b.bc,ff_h264_cabac_tables,pl
+// RUN: llvm-dis < %t/out.2.2.internalize.bc | FileCheck %s
+
+//--- a.c
+/// IR symtab does not track inline asm symbols, so we don't know
+/// ff_h264_cabac_tables is undefined.
+// NM-NOT: {{.}}
+// NM:     ---------------- T ref
+// NM-NOT: {{.}}
+const char *ref() {
+  const char *ret;
+  asm("lea ff_h264_cabac_tables(%%rip), %0" : "=r"(ret));
+  return ret;
+}
+
+//--- b.c
+/// ff_h264_cabac_tables has __attribute__((used)) in the source code, which means
+/// its definition must be retained because there can be references the compiler
+/// cannot see (inline asm reference). Test we don't internalize it.
+// CHECK: @ff_h264_cabac_tables = dso_local constant [1 x i8] c"\09"
+__attribute__((used))
+const char ff_h264_cabac_tables[1] = "\x09";

llvm/lib/LTO/LTO.cpp

if (Res.Prevailing) {
       // the same symbol we want to use IR name of the prevailing symbol.
       // Otherwise, if we haven't seen a prevailing symbol, set the name so that
       // we can later use it to check if there is any prevailing copy in IR.
       GlobalRes.IRName = std::string(Sym.getIRName());
     }
 
     // Set the partition to external if we know it is re-defined by the linker
     // with -defsym or -wrap options, used elsewhere, e.g. it is visible to a
-    // regular object, is referenced from llvm.compiler_used, or was already
-    // recorded as being referenced from a different partition.
+    // regular object, is referenced from llvm.compiler.used/llvm.used, or was
+    // already recorded as being referenced from a different partition.
     if (Res.LinkerRedefined || Res.VisibleToRegularObj || Sym.isUsed() ||
         (GlobalRes.Partition != GlobalResolution::Unknown &&
          GlobalRes.Partition != Partition)) {
       GlobalRes.Partition = GlobalResolution::External;
     } else
       // First recorded reference, save the current partition.
       GlobalRes.Partition = Partition;
 

llvm/lib/Object/IRSymtab.cpp

struct Builder {
   Error build(ArrayRef<Module *> Mods);
 };
 
 Error Builder::addModule(Module *M) {
   if (M->getDataLayoutStr().empty())
     return make_error<StringError>("input module has no datalayout",
                                    inconvertibleErrorCode());
 
+  // Symbols in the llvm.used list will get the FB_Used bit and will not be
+  // internalized. We do this for llvm.compiler.used as well:
+  //
+  // IR symbol table tracks module-level asm symbol references but not inline
+  // asm, so we may not know that a symbol defined in another translation unit
+  // is referenced. That definition may have __attribute__((used)) (which lowers
+  // to llvm.compiler.used on ELF targets) to communicate to the compiler that
+  // it may be used by inline asm. The usage is perfectly fine, so we treat
+  // llvm.compiler.used conservatively as llvm.used to work around our own
+  // limitation.
   SmallVector<GlobalValue *, 4> UsedV;
   collectUsedGlobalVariables(*M, UsedV, /*CompilerUsed=*/false);
+  collectUsedGlobalVariables(*M, UsedV, /*CompilerUsed=*/true);
   SmallPtrSet<GlobalValue *, 4> Used(UsedV.begin(), UsedV.end());
 
   ModuleSymbolTable Msymtab;
   Msymtab.addModule(M);
 
   storage::Module Mod;
   Mod.Begin = Syms.size();
   Mod.End = Syms.size() + Msymtab.symbols().size();

llvm/test/ThinLTO/X86/asm.ll

+; RUN: split-file %s %t
+; RUN: opt -module-summary %t/a.s -o %t/a.bc
+; RUN: opt -module-summary %t/b.s -o %t/b.bc
+; RUN: llvm-nm %t/a.bc | FileCheck %s --check-prefix=NM
+
+; RUN: llvm-lto2 run %t/a.bc %t/b.bc -o %t/out -save-temps -r=%t/a.bc,ref,plx -r=%t/b.bc,ff_h264_cabac_tables,pl
+; RUN: llvm-dis < %t/out.2.2.internalize.bc | FileCheck %s
+
+;--- a.s
+;; IR symtab does not track inline asm symbols, so we don't know
+;; ff_h264_cabac_tables is undefined.
+; NM-NOT: {{.}}
+; NM:     ---------------- T ref
+; NM-NOT: {{.}}
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i8* @ref() {
+entry:
+  %0 = tail call i8* asm sideeffect "lea ff_h264_cabac_tables(%rip), $0", "=&r,~{dirflag},~{fpsr},~{flags}"()
+  ret i8* %0
+}
+
+;--- b.s
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+;; ff_h264_cabac_tables has __attribute__((used)) in the source code, which means
+;; its definition must be retained because there can be references the compiler
+;; cannot see (inline asm reference). Test we don't internalize it.
+; CHECK: @ff_h264_cabac_tables = dso_local constant [1 x i8] c"\09"
+@ff_h264_cabac_tables = dso_local constant [1 x i8] c"\09"
+
[e-mail] = appending global [1 x i8*] [i8* bitcast ([1 x i8]* @ff_h264_cabac_tables to i8*)], section "llvm.metadata"

llvm/test/tools/gold/X86/emit-llvm.ll

 @g8 = external global i32
 
 ; CHECK-DAG: define internal void @f1()
 ; OPT2-NOT: @f1
 define hidden void @f1() {
   ret void
 }
 
+;; f2 is not internalized because it is in the llvm.used list.
 ; CHECK-DAG: define hidden void @f2()
 ; OPT-DAG: define hidden void @f2()
 define hidden void @f2() {
   ret void
 }
 
 @llvm.used = appending global [1 x i8*] [ i8* bitcast (void ()* @f2 to i8*)]
 

 define i32* @f7() {
   ret i32* @g7
 }
 
 define i32* @f8() {
   ret i32* @g8
 }
 
+;; f9 is not internalized because it is in the llvm.compiler.used list.
+; CHECK-DAG: define hidden void @f9()
+; OPT-DAG: define hidden void @f9()
+define hidden void @f9() {
+  ret void
+}
+
[e-mail] = appending global [1 x i8*] [ i8* bitcast (void ()* @f9 to i8*)]
+
 ; RES: .o,f1,pl{{$}}
 ; RES: .o,f2,pl{{$}}
 ; RES: .o,f3,px{{$}}
 ; RES: .o,f4,p{{$}}
 ; RES: .o,f5,px{{$}}
 ; RES: .o,f6,p{{$}}
 ; RES: .o,f7,px{{$}}
 ; RES: .o,f8,px{{$}}
+; RES: .o,f9,pl{{$}}
 ; RES: .o,g1,px{{$}}
 ; RES: .o,g2,p{{$}}
 ; RES: .o,g3,p{{$}}
 ; RES: .o,g4,px{{$}}
 ; RES: .o,g5,px{{$}}
 ; RES: .o,g6,p{{$}}
 ; RES: .o,g7,{{$}}
 ; RES: .o,g8,{{$}}
 ; RES: .o,g9,px{{$}}
 ; RES: .o,g10,px{{$}}