[LTO][codegen] Add TargetLibraryInfoWrapperPass initially

[FreddyYe]

Summary

Many codegen pass require this pass with useful triple info. Legacy pass manager need to
add a TargetLibraryInfo with the module info before run passes. Or the TargetLibraryInfo
will be initialized too conservative.

[FreddyYe]

CodeGenPasses is a legacy pass manager here. For now. llc.cpp has not switched to the new pass manager. llc.cpp does add this pass. And many passes in codegen need a right TargetLibraryInfo. If not adding it here with the module's triple info. TargetLibraryInfo will be initialized with a null string. That results in setting many library calls unavailable, as conservative as possible. I think it's not reasonable. Welcome discuss! Welcome adding more reviewers.

[pengfei]

Not sure if it affects the complie time. +@nikic to have a look.

[nikic]

@pengfei I don't think this will affect compile-time, the TLI pass should be "free".

[pengfei]

In D115850#3199730, @nikic wrote:

@pengfei I don't think this will affect compile-time, the TLI pass should be "free".

Got it. Thank you @nikic !

[pengfei]

I think this should be good to go.

[aeubanks]

the summary needs more description, why is this change needed? is it fixing a bug? a test case would be nice

[FreddyYe]

For example this patch can fix such performance drop on Linux:

$ cat foo.h                                                                                                                                                                                                            
double foo(double a);
$ cat foo.c                                                                                                                                                                                                            
#include <math.h>
#include "foo.h"

double foo(double a){
    return sqrt(a);
}
$ cat main.c
#include <stdio.h>
#include "foo.h"
int main() { printf("%lf\n", foo(200));}
$ cat foo.sh                                                                                                                                                                                                           
clang -c -o foo.o -Ofast -flto foo.c                                                                                                                                                                                                                                    
clang -c -o main.o -Ofast -flto main.c                                                                                                                                                                                                                                  
clang -Ofast -flto foo.o main.o -lm                                                                                                                                                                                                                                     
objdump -d a.out | less
$ sh foo.sh

Without this patch, when enable -flto, it will generate call __sqrt_finite, when disable -flto , it will generate sqrtsd %xmm0,%xmm0. The later version is faster. I don't know how to add lto dependent lit test. Hi @aeubanks, do you know how?

[craig.topper]

In D115850#3202076, @FreddyYe wrote:

For example this patch can fix such performance drop on Linux:

$ cat foo.h                                                                                                                                                                                                            
double foo(double a);
$ cat foo.c                                                                                                                                                                                                            
#include <math.h>
#include "foo.h"

double foo(double a){
    return sqrt(a);
}
$ cat main.c
#include <stdio.h>
#include "foo.h"
int main() { printf("%lf\n", foo(200));}
$ cat foo.sh                                                                                                                                                                                                           
clang -c -o foo.o -Ofast -flto foo.c                                                                                                                                                                                                                                    
clang -c -o main.o -Ofast -flto main.c                                                                                                                                                                                                                                  
clang -Ofast -flto foo.o main.o -lm                                                                                                                                                                                                                                     
objdump -d a.out | less
$ sh foo.sh

Without this patch, when enable -flto, it will generate call __sqrt_finite, when disable -flto , it will generate sqrtsd %xmm0,%xmm0. The later version is faster. I don't know how to add lto dependent lit test. Hi @aeubanks, do you know how?

Why would TLI be involved in that test. Isn't the sqrt call converted to llvm.sqrt by the frontend?

[FreddyYe]

Why would TLI be involved in that test. Isn't the sqrt call converted to llvm.sqrt by the frontend?

Sorry for the late response. BTW, thanks for review! This maybe not a good example. I realized it depends on certain version of math.h. Here's the source code pre-processed on my system which can generate call __sqrt_finite: https://gcc.godbolt.org/z/947nhffv1

[FreddyYe]

ping...

[aeubanks]

I'd expect most/all the TLI optimizations to have happened before codegen, although some IR passes can be added to the codegen pipeline, and having the right TLI for those does make sense.
it'd be nice to have an actual test for this in tree, but that might be hard to come up with. can you use --print-after-all to see which pass is doing something different when this patch is applied, just to convince me that this actually works?

[craig.topper]

In D115850#3223072, @aeubanks wrote:

I'd expect most/all the TLI optimizations to have happened before codegen, although some IR passes can be added to the codegen pipeline, and having the right TLI for those does make sense.
it'd be nice to have an actual test for this in tree, but that might be hard to come up with. can you use --print-after-all to see which pass is doing something different when this patch is applied, just to convince me that this actually works?

Speculating a little. For the sqrt_finite example given previously, it's probably SelectionDAGBuilder that is doing something different. There is code in there to convert LibFunc_sqrt_finite to ISD::FSQRT. I guess we don't turn finite calls into intrinsics anywhere before that. TargetLibraryInfo needs the Triple in order to know if sqrt_finite is supported. It's marked unavailable in TargetLibraryInfo.cpp's initialize function unless the Triple has a Linux OS and a GNU environment.

[aeubanks]

In D115850#3223213, @craig.topper wrote:

In D115850#3223072, @aeubanks wrote:

I'd expect most/all the TLI optimizations to have happened before codegen, although some IR passes can be added to the codegen pipeline, and having the right TLI for those does make sense.
it'd be nice to have an actual test for this in tree, but that might be hard to come up with. can you use --print-after-all to see which pass is doing something different when this patch is applied, just to convince me that this actually works?

Speculating a little. For the sqrt_finite example given previously, it's probably SelectionDAGBuilder that is doing something different. There is code in there to convert LibFunc_sqrt_finite to ISD::FSQRT. I guess we don't turn finite calls into intrinsics anywhere before that. TargetLibraryInfo needs the Triple in order to know if sqrt_finite is supported. It's marked unavailable in TargetLibraryInfo.cpp's initialize function unless the Triple has a Linux OS and a GNU environment.

in that case we should be able to put some IR with some target triple through the LTO pipeline and see that we call the proper function as a test

[craig.topper]

Commandeering to add test case.

[craig.topper]

Add LTO test.

[aeubanks]

awesome, thanks!

[FreddyYe]

Thank you both! Your speculating is right! That's the original motivation for this patch.

[craig.topper]

@FreddyYe do you mind taking this back to commit since the code change is yours.

[FreddyYe]

In D115850#3223925, @craig.topper wrote:

@FreddyYe do you mind taking this back to commit since the code change is yours.

Thanks for asking. I don't mind! pls commit!

[craig.topper]

In D115850#3223946, @FreddyYe wrote:

In D115850#3223925, @craig.topper wrote:

@FreddyYe do you mind taking this back to commit since the code change is yours.

Thanks for asking. I don't mind! pls commit!

Sorry. I was asking you to take ownership back and commit it yourself.

[FreddyYe]

All right. I'll commit then.

llvm/lib/LTO/LTOBackend.cpp

static void codegen(const Config &Conf, TargetMachine *TM,
 
   Expected<std::unique_ptr<CachedFileStream>> StreamOrErr = AddStream(Task);
   if (Error Err = StreamOrErr.takeError())
     report_fatal_error(std::move(Err));
   std::unique_ptr<CachedFileStream> &Stream = *StreamOrErr;
   TM->Options.ObjectFilenameForDebug = Stream->ObjectPathName;
 
   legacy::PassManager CodeGenPasses;
+  TargetLibraryInfoImpl TLII(Triple(Mod.getTargetTriple()));
+  CodeGenPasses.add(new TargetLibraryInfoWrapperPass(TLII));
   CodeGenPasses.add(
       createImmutableModuleSummaryIndexWrapperPass(&CombinedIndex));
   if (Conf.PreCodeGenPassesHook)
     Conf.PreCodeGenPassesHook(CodeGenPasses);
   if (TM->addPassesToEmitFile(CodeGenPasses, *Stream->OS,
                               DwoOut ? &DwoOut->os() : nullptr,
                               Conf.CGFileType))
     report_fatal_error("Failed to setup codegen");

llvm/test/LTO/X86/tli-sqrtf_finite.ll

+; RUN: llvm-as < %s > %t
+; RUN: llvm-lto %t -exported-symbol=foo -filetype=asm -o - | FileCheck %s
+
+; Check that sqrtf_finite is recognized as a libcall by SelectionDAGBuilder
+; to enable sqrtss instruction to be used.
+
+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 float @foo(float %x) {
+; CHECK: sqrtss
+  %call = tail call nnan ninf float @__sqrtf_finite(float %x) readnone
+  ret float %call
+}
+
+declare float @__sqrtf_finite(float) readnone