[LTO] Account for overriding lib calls via the alias attribute

[wristow]

Summary

Given a library call that is represented as an llvm intrinsic call, but later transformed to an actual call, if an overriding definition of that library routine is provided indirectly via an alias, verify that LTO does not eliminate the definition.

This is a proposed fix for PR38547.

[tejohnson]

I'll respond more on the bug (especially since I can't reproduce the actual link time error), but I think the right way to fix these types of issues is to compile with one of -ffreestanding, -fno-builtin, or -fno-builtin-log (or whatever is being overridden), so that clang doesn't translate the call to an intrinsic. Clang translates to the intrinsic assuming it is calling the builtin, which means it can assume specific semantics for the call, and there is nothing stopping the compiler from translating that intrinsic call to e.g. an inline expansion of the function (that currently happens e.g. for memcpy and some other builtins).

[wristow]

In D52836#1256581, @tejohnson wrote:

I'll respond more on the bug (especially since I can't reproduce the actual link time error), ...

Thanks for that. I've responded more thoroughly in the bug, but a short note here to clarify.

The reason the link-time error wasn't reproducible appears to be a new LTO API vs old LTO API issue (as you suggested in the PR). Also, I agree that your point about -fno-builtin etc. means clang can use the intrinsics, and thus the user cannot depend on the overriding happening.

So in a sense, it comes down to whether we want to make the old LTO API behave like the new one does for situations like this. If the answer is no, then I'll abandon this patch. If the answer is yes, then this appears to work, but maybe there is a better way (e.g., a way for the old API to be driven to make the link pass).

[tejohnson]

As discussed on bug, this is already fixed in new LTO API so best to be consistent, regardless of whether users should build with -fno-builtins.

Lgtm

[MaskRay]

The check lines are:

; Check that the IR contains the overriding definition of the library routine
; in the IR after LTO:
; CHECK_IR: define internal float @logf(float [[X:%.*]])
; CHECK_IR-NEXT:   [[TMP:%.*]] = fadd float [[X]], [[X]]
; CHECK_IR-NEXT:   ret float [[TMP]]

However, the function argument uses an implicit label %0, causing failure of LTO/X86/libcall-overridden-via-alias.ll

; Function Attrs: norecurse nounwind readnone
define internal float @logf(float) #2 {
  %2 = fadd float %0, %0
  ret float %2
}

[wristow]

In D52836#1262070, @MaskRay wrote:

However, the function argument uses an implicit label %0, causing failure of LTO/X86/libcall-overridden-via-alias.ll

; Function Attrs: norecurse nounwind readnone
define internal float @logf(float) #2 {
  %2 = fadd float %0, %0
  ret float %2
}

I'm not seeing the implicit %0 in local runs, so I'm not seeing this failure. And searching on the bots I couldn't find it either. What I see in my local runs is an explicit parameter %x:

; Function Attrs: norecurse nounwind readnone
define internal float @logf(float %x) #2 {
  %add = fadd float %x, %x
  ret float %add
}

I can change the test to just look for an fadd followed by the ret (and so just not verify that the fadd is adding the formal arg to itself, which isn't critical), but I'm puzzled as to how this is happening. Can you let me know which bot is failing?

[steven_wu]

You need to do a release build without assertion. Local names like parameters are dropped from IR to save space/memory.
I think you just need to check the parameter name or the implementation in your test case.

[ioeric]

Hi Warren, I'm also hitting the same error in our internal test with
optimized build. Would you mind relaxing the checks as you described?
Thanks!

[wristow]

Thanks for explaining, and sorry for the trouble!
Test updated at r344286.

lib/LTO/UpdateCompilerUsed.cpp

void findLibCallsAndAsm(GlobalValue &GV) {
     // There are no restrictions to apply to declarations.
     if (GV.isDeclaration())
       return;
 
     // There is nothing more restrictive than private linkage.
     if (GV.hasPrivateLinkage())
       return;
 
-    // Conservatively append user-supplied runtime library functions to
-    // llvm.compiler.used.  These could be internalized and deleted by
-    // optimizations like -globalopt, causing problems when later optimizations
-    // add new library calls (e.g., llvm.memset => memset and printf => puts).
+    // Conservatively append user-supplied runtime library functions (supplied
+    // either directly, or via a function alias) to llvm.compiler.used.  These
+    // could be internalized and deleted by optimizations like -globalopt,
+    // causing problems when later optimizations add new library calls (e.g.,
+    // llvm.memset => memset and printf => puts).
     // Leave it to the linker to remove any dead code (e.g. with -dead_strip).
-    if (isa<Function>(GV) && Libcalls.count(GV.getName())) {
+    GlobalValue *FuncAliasee = nullptr;
+    if (isa<GlobalAlias>(GV)) {
+      auto *A = cast<GlobalAlias>(&GV);
+      FuncAliasee = dyn_cast<Function>(A->getAliasee());
+    }
+    if ((isa<Function>(GV) || FuncAliasee) && Libcalls.count(GV.getName())) {
       LLVMUsed.push_back(&GV);
       return;
     }
 
     SmallString<64> Buffer;
     TM.getNameWithPrefix(Buffer, &GV, Mangler);
     if (AsmUndefinedRefs.count(Buffer))
       LLVMUsed.push_back(&GV);

test/LTO/X86/libcall-overridden-via-alias.ll

+; Given a library call that is represented as an llvm intrinsic call, but
+; later transformed to an actual call, if an overriding definition of that
+; library routine is provided indirectly via an alias, verify that LTO
+; does not eliminate the definition.  This is a test for PR38547.
+;
+; RUN: llvm-as -o %t1 %s
+; RUN: llvm-lto -exported-symbol=main -save-merged-module -filetype=asm -o %t2 %t1
+; RUN: llvm-dis -o - %t2.merged.bc | FileCheck --check-prefix=CHECK_IR %s
+;
+; Check that the call is represented as an llvm intrinsic in the IR after LTO:
+; CHECK_IR-LABEL: main
+; CHECK_IR: call float @llvm.log.f32
+;
+; Check that the IR contains the overriding definition of the library routine
+; in the IR after LTO:
+; CHECK_IR: define internal float @logf(float [[X:%.*]])
+; CHECK_IR-NEXT:   [[TMP:%.*]] = fadd float [[X]], [[X]]
+; CHECK_IR-NEXT:   ret float [[TMP]]
+;
+; Check that the assembly code from LTO contains the call to the expected
+; library routine, and that the overriding definition of the library routine
+; is present:
+; RUN: FileCheck --check-prefix=CHECK_ASM %s < %t2
+; CHECK_ASM-LABEL: main:
+; CHECK_ASM: callq logf
+; CHECK_ASM-LABEL: logf:
+; CHECK_ASM-NEXT: add
+; CHECK_ASM-NEXT: ret
+
+; Produced from the following source-code:
+;
+;extern float logf(float);
+;// 'src' and 'dst' are 'volatile' to prohibit optimization.
+;volatile float src = 3.14f;
+;volatile float dst;
+;
+;int main() {
+;  dst = logf(src);
+;  return 0;
+;}
+;
+;extern float fname(float x);
+;float fname(float x) {
+;  return x + x;
+;}
+;
+;float logf(float x) __attribute__((alias("fname")));
+;
+target triple = "x86_64-unknown-linux-gnu"
+
+@src = global float 0x40091EB860000000, align 4
+@dst = common global float 0.000000e+00, align 4
+
+@logf = alias float (float), float (float)* @fname
+
+define i32 @main() local_unnamed_addr {
+entry:
+  %0 = load volatile float, float* @src, align 4
+  %1 = tail call float @llvm.log.f32(float %0)
+  store volatile float %1, float* @dst, align 4
+  ret i32 0
+}
+
+declare float @llvm.log.f32(float)
+
+define float @fname(float %x) {
+  %add = fadd float %x, %x
+  ret float %add
+}