[ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends

[tejohnson]

Summary

Adds information used when creating the TargetLibraryInfoImpl as module flags.
Use these module flags to create a TargetLibraryInfoImpl in LTO backends.
Supports handling of -fno-builtin* and -fveclib options passed to distributed
backend clang invocations.

[tejohnson]

ping

[dexonsmith]

I wonder if we could add a module flag for the TLI, and then store that in the summary. Would it work for both implementations in that case?

[tejohnson]

In D60162#1472085, @dexonsmith wrote:

I wonder if we could add a module flag for the TLI, and then store that in the summary. Would it work for both implementations in that case?

A module flag summarizing the TLI should theoretically work (we wouldn't need to put anything in the ThinLTO summary, just needs to be in the IR for the backends). There is a lot of stuff in the TLI though, it would all need to be summarized.

[hfinkel]

In D60162#1472147, @tejohnson wrote:

In D60162#1472085, @dexonsmith wrote:

I wonder if we could add a module flag for the TLI, and then store that in the summary. Would it work for both implementations in that case?

A module flag summarizing the TLI should theoretically work (we wouldn't need to put anything in the ThinLTO summary, just needs to be in the IR for the backends). There is a lot of stuff in the TLI though, it would all need to be summarized.

Regarding a summary, I believe that TLI's only inputs (currently) are the target triple, the list of disabled functions (or the setting to disable all functions), and the kind of vector functions enabled.

[tejohnson]

Working on this one again as it previously wasn't causing any issues but just now got exposed in multiple places and started causing correctness issues. Two questions:

1. What should be the behavior when merging modules e.g. for LTO. I'm thinking of the following, with multiple module flags to specify different aspects:
2. For the NoBuiltinFuncs I'm thinking of a module flag containing the list of strings, with AppendUnique merging behavior, so we get a superset in the merged module
3. For the flag to disable all builtins, have a separate module flag with a Max merge (so we get the most conservative behavior)
4. For the VectorLibrary, not sure - should it be an Error to merge modules with different libraries?
5. While we get the above hammered out, would it be ok to submit this one (and companion clang change) to fix the bug (then remove when the module flags are added)?

[tejohnson]

In D60162#1498288, @tejohnson wrote:

Working on this one again as it previously wasn't causing any issues but just now got exposed in multiple places and started causing correctness issues. Two questions:

1. What should be the behavior when merging modules e.g. for LTO. I'm thinking of the following, with multiple module flags to specify different aspects: a) For the NoBuiltinFuncs I'm thinking of a module flag containing the list of strings, with AppendUnique merging behavior, so we get a superset in the merged module b) For the flag to disable all builtins, have a separate module flag with a Max merge (so we get the most conservative behavior) c) For the VectorLibrary, not sure - should it be an Error to merge modules with different libraries?
2. While we get the above hammered out, would it be ok to submit this one (and companion clang change) to fix the bug (then remove when the module flags are added)?

Weird, phabricator added numbering to all of my bullets, which formatted very differently than intended. Modified the above a bit to hopefully format better (2 high level questions, and some subquestions on the first one).

[tejohnson]

Rework using module flags.

[tejohnson]

In D60162#1498392, @tejohnson wrote:

In D60162#1498288, @tejohnson wrote:

Working on this one again as it previously wasn't causing any issues but just now got exposed in multiple places and started causing correctness issues. Two questions:

1. What should be the behavior when merging modules e.g. for LTO. I'm thinking of the following, with multiple module flags to specify different aspects: a) For the NoBuiltinFuncs I'm thinking of a module flag containing the list of strings, with AppendUnique merging behavior, so we get a superset in the merged module b) For the flag to disable all builtins, have a separate module flag with a Max merge (so we get the most conservative behavior) c) For the VectorLibrary, not sure - should it be an Error to merge modules with different libraries?
2. While we get the above hammered out, would it be ok to submit this one (and companion clang change) to fix the bug (then remove when the module flags are added)?

Weird, phabricator added numbering to all of my bullets, which formatted very differently than intended. Modified the above a bit to hopefully format better (2 high level questions, and some subquestions on the first one).

I went ahead and implemented the above approach, PTAL. I added test cases for ensuring that the various clang driver flags set up the module flags properly, that merging behaves as expected, and that these options correctly affect LTO backends.

Note that this patch now includes both clang and llvm changes. I am going to mark the old clang child patch as obsolete.

[steven_wu]

Thanks for doing this. I think module flag is a good idea. Some comments inline.

clang/test/CodeGen/svml-calls.ll

16

Personally, I think codegen tests like this will be cleaner to keep in LLVM. Clang tests just test the IRGen of the module flag and LLVM tests check that those flags are respected and module flag merge is respected.

llvm/lib/LTO/LTOBackend.cpp

221

Should this be done not just for LTOBackend but for regular compilation as well? LegacyCodegenerator and llc can all be benefit from a matching TargetLibraryInfo?

[tejohnson]

clang/test/CodeGen/svml-calls.ll

16

Ok. I originally was doing it here to ensure that ThinLTO distributed backends (which use clang) are fixed. But now that the approach no longer involves passing down additional info via that path into LTO, I don't think we need to test this explicitly but rather just as an LLVM LTO test. Will move.

llvm/lib/LTO/LTOBackend.cpp

221

Yeah, probably. I think the best way to have this utilized everywhere is to move the below code into the TargetLibraryInfoImpl itself - by having it also take the Module as a parameter). Probably as a required parameter, to ensure it is used consistently. WDYT?

[tejohnson]

llvm/lib/LTO/LTOBackend.cpp

221

I meant, "into the TargetLibraryInfoImpl constructor"

[steven_wu]

llvm/lib/LTO/LTOBackend.cpp

221

SGTM

[tejohnson]

There is discussion of using function attributes to control this instead, see https://reviews.llvm.org/D61634.
I'll hold off on making the planned changes here until the overall approach is decided.

[tejohnson]

As per D61634, we are going to use function attributes instead.

clang/lib/CodeGen/CodeGenModule.cpp

void CodeGenModule::Release() {
 
   if (CodeGenOpts.CFProtectionBranch &&
       Target.checkCFProtectionBranchSupported(getDiags())) {
     // Indicate that we want to instrument branch control flow protection.
     getModule().addModuleFlag(llvm::Module::Override, "cf-protection-branch",
                               1);
   }
 
+  if (!CodeGenOpts.SimplifyLibCalls) {
+    // Indicate that all built in functions are disabled.
+    getModule().addModuleFlag(llvm::Module::Max, "DisableAllBuiltins", 1);
+  }
+
+  if (!CodeGenOpts.getNoBuiltinFuncs().empty()) {
+    std::vector<llvm::Metadata *> Nodes;
+    for (auto &FuncName : CodeGenOpts.getNoBuiltinFuncs())
+      Nodes.push_back(llvm::MDString::get(VMContext, FuncName));
+    getModule().addModuleFlag(llvm::Module::AppendUnique, "NoBuiltins",
+                              llvm::MDNode::get(VMContext, Nodes));
+  }
+
+  switch (CodeGenOpts.getVecLib()) {
+  case CodeGenOptions::Accelerate:
+    getModule().addModuleFlag(llvm::Module::Error, "VectorLibrary",
+                              llvm::MDString::get(VMContext, "Accelerate"));
+    break;
+  case CodeGenOptions::SVML:
+    getModule().addModuleFlag(llvm::Module::Error, "VectorLibrary",
+                              llvm::MDString::get(VMContext, "SVML"));
+    break;
+  default:
+    break;
+  }
+
   if (LangOpts.CUDAIsDevice && getTriple().isNVPTX()) {
     // Indicate whether __nvvm_reflect should be configured to flush denormal
     // floating point values to 0.  (This corresponds to its "__CUDA_FTZ"
     // property.)
     getModule().addModuleFlag(llvm::Module::Override, "nvvm-reflect-ftz",
                               CodeGenOpts.FlushDenorm ? 1 : 0);
   }
 

clang/test/CodeGen/nobuiltins.c

+// RUN: %clang_cc1 -emit-llvm -ffreestanding %s -o - | FileCheck %s -check-prefix=DISABLEALL
+// RUN: %clang_cc1 -emit-llvm -fno-builtin %s -o - | FileCheck %s -check-prefix=DISABLEALL
+// RUN: %clang_cc1 -emit-llvm -fno-builtin-memset -fno-builtin-memcpy %s -o - | FileCheck %s -check-prefix=DISABLESOME
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s -check-prefix=NONE
+
+// ACCELERATE-DAG: !{{[0-9]+}} = !{i32 1, !"VectorLibrary", !"Accelerate"}
+// DISABLEALL-DAG: !{{[0-9]+}} = !{i32 7, !"DisableAllBuiltins", i32 1}
+// DISABLESOME-DAG: !{{[0-9]+}} = !{i32 6, !"NoBuiltins", ![[LIST:[0-9]+]]
+// DISABLESOME-DAG: ![[LIST]] = !{!"memset", !"memcpy"}
+// DISABLEMEMSET-DAG: !{{[0-9]+}} = !{i32 6, !"NoBuiltins", ![[LIST:[0-9]+]]
+// DISABLEMEMSET-DAG: ![[LIST]] = !{!"memset"}
+// NONE-NOT: Builtins
+
+// Now ensure merging gets the expected behavior
+// RUN: %clang -c -flto %s -o %t0.o
+// RUN: %clang -c -flto -fno-builtin %s -o %t1.o
+// RUN: %clang -c -flto -fno-builtin-memset %s -o %t2.o
+// RUN: %clang -c -flto -fno-builtin-memcpy %s -o %t3.o
+// Merge none with -fno-builtin -> DisableAllBuiltins set
+// RUN: llvm-lto %t0.o %t1.o -o %t4.o -save-merged-module
+// RUN: llvm-dis %t4.o.merged.bc -o - | FileCheck %s --check-prefix=DISABLEALL
+// Merge -fno-builtin -fno-builtin-memset -> Both DisableAllBuiltins set and
+// NoBuiltins=memset.
+// RUN: llvm-lto %t1.o %t2.o -o %t4.o -save-merged-module
+// RUN: llvm-dis %t4.o.merged.bc -o - | FileCheck %s --check-prefix=DISABLEALL --check-prefix=DISABLEMEMSET
+// Merge -fno-builtin-memset -fno-builtin-memcpy -> NoBuiltins=memset,memcpy.
+// RUN: llvm-lto %t2.o %t3.o -o %t4.o -save-merged-module
+// RUN: llvm-dis %t4.o.merged.bc -o - | FileCheck %s --check-prefix=DISABLESOME

clang/test/CodeGen/svml-calls.ll

+; Test to ensure that -fveclib=SVML module flag is handled properly in
+; the ThinLTO distributed backend.
+
+; RUN: opt -module-summary -o %t.o %s
+; RUN: llvm-lto -thinlto -o %t %t.o
+; RUN: %clang -target x86_64-unknown-linux-gnu -O3 -o %t2.o -x ir %t.o -c -fthinlto-index=%t.thinlto.bc -save-temps=obj -mllvm -force-vector-width=4 -mllvm -force-vector-interleave=1 -mavx
+; RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck %s
+
+; RUN opt -vector-library=SVML -loop-vectorize -force-vector-width=4 -force-vector-interleave=1 -mattr=avx -S < %s | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare double @sin(double) #0
+
+define void @sin_f64(double* nocapture %varray) {
+; CHECK-LABEL: @sin_f64(
+; CHECK:    [[TMP5:%.*]] = call <4 x double> @__svml_sin4(<4 x double> [[TMP4:%.*]])
+; CHECK:    ret void
+;
+entry:
+  br label %for.body
+
+for.body:
+  %iv = phi i64 [ 0, %entry ], [ %iv.next, %for.body ]
+  %tmp = trunc i64 %iv to i32
+  %conv = sitofp i32 %tmp to double
+  %call = tail call double @sin(double %conv)
+  %arrayidx = getelementptr inbounds double, double* %varray, i64 %iv
+  store double %call, double* %arrayidx, align 4
+  %iv.next = add nuw nsw i64 %iv, 1
+  %exitcond = icmp eq i64 %iv.next, 1000
+  br i1 %exitcond, label %for.end, label %for.body
+
+for.end:
+  ret void
+}
+
+!llvm.module.flags = !{!1}
+!1 = !{i32 1, !"VectorLibrary", !"SVML"}

clang/test/CodeGen/thinlto_backend_nobuiltin.ll

+; Make sure that -fno-builtin/-ffreestanding module flag is handled properly in
+; the ThinLTO distributed backend.
+; REQUIRES: x86-registered-target
+
+; RUN: opt -module-summary -o %t.o %s
+; RUN: llvm-lto -thinlto -o %t %t.o
+
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t2.o -x ir %t.o -c -fthinlto-index=%t.thinlto.bc -save-temps=obj
+; RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck %s --check-prefix=NOBUILTIN
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i8* @memset(i8*, i8, i64)
+
+define i8* @test(i8* %mem, i8 %val, i64 %size) {
+  ; NOBUILTIN: call i8* @memset
+  %ret = call i8* @memset(i8* %mem, i8 %val, i64 %size)
+  ret i8* %ret
+}
+
+!llvm.module.flags = !{!1}
+!1 = !{i32 7, !"DisableAllBuiltins", i32 1}

clang/test/CodeGen/thinlto_backend_nobuiltin_memset.ll

+; Make sure that -fno-builtin-memset module flag is handled properly in
+; the ThinLTO distributed backend.
+; REQUIRES: x86-registered-target
+
+; RUN: opt -module-summary -o %t.o %s
+; RUN: llvm-lto -thinlto -o %t %t.o
+
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t2.o -x ir %t.o -c -fthinlto-index=%t.thinlto.bc -save-temps=obj
+; RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck %s --check-prefix=NOBUILTIN
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i8* @memset(i8*, i8, i64)
+
+define i8* @test(i8* %mem, i8 %val, i64 %size) {
+  ; NOBUILTIN: call i8* @memset
+  %ret = call i8* @memset(i8* %mem, i8 %val, i64 %size)
+  ret i8* %ret
+}
+
+!llvm.module.flags = !{!1}
+!1 = !{i32 6, !"NoBuiltins", !2}
+!2 = !{!"memset"}
+

clang/test/CodeGen/veclib-calls.ll

+; Test to ensure that -fveclib=Accelerate module flag is handled properly in
+; the ThinLTO distributed backend.
+
+; RUN: opt -module-summary -o %t.o %s
+; RUN: llvm-lto -thinlto -o %t %t.o
+; RUN: %clang -target x86_64-unknown-linux-gnu -O3 -o %t2.o -x ir %t.o -c -fthinlto-index=%t.thinlto.bc -save-temps=obj
+; RUN: llvm-dis %t.s.4.opt.bc -o - | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+;CHECK-LABEL: @sqrt_f32(
+;CHECK: vsqrtf{{.*}}<4 x float>
+;CHECK: ret void
+declare float @sqrtf(float) nounwind readnone
+define void @sqrt_f32(i32 %n, float* noalias %y, float* noalias %x) nounwind uwtable {
+entry:
+  %cmp6 = icmp sgt i32 %n, 0
+  br i1 %cmp6, label %for.body, label %for.end
+
+for.body:                                         ; preds = %entry, %for.body
+  %indvars.iv = phi i64 [ %indvars.iv.next, %for.body ], [ 0, %entry ]
+  %arrayidx = getelementptr inbounds float, float* %y, i64 %indvars.iv
+  %0 = load float, float* %arrayidx, align 4
+  %call = tail call float @sqrtf(float %0) nounwind readnone
+  %arrayidx2 = getelementptr inbounds float, float* %x, i64 %indvars.iv
+  store float %call, float* %arrayidx2, align 4
+  %indvars.iv.next = add i64 %indvars.iv, 1
+  %lftr.wideiv = trunc i64 %indvars.iv.next to i32
+  %exitcond = icmp eq i32 %lftr.wideiv, %n
+  br i1 %exitcond, label %for.end, label %for.body
+
+for.end:                                          ; preds = %for.body, %entry
+  ret void
+}
+
+!llvm.module.flags = !{!1}
+!1 = !{i32 1, !"VectorLibrary", !"Accelerate"}

clang/test/CodeGen/veclib.c

+// RUN: %clang_cc1 -emit-llvm -fveclib=Accelerate %s -o - | FileCheck %s -check-prefix=ACCELERATE
+// RUN: %clang_cc1 -emit-llvm -fveclib=SVML %s -o - | FileCheck %s -check-prefix=SVML
+// RUN: %clang_cc1 -emit-llvm -fveclib=none %s -o - | FileCheck %s -check-prefix=NONE
+// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s -check-prefix=NONE
+
+// ACCELERATE: !{{[0-9]+}} = !{i32 1, !"VectorLibrary", !"Accelerate"}
+// SVML: !{{[0-9]+}} = !{i32 1, !"VectorLibrary", !"SVML"}
+// NONE-NOT: VectorLibrary
+
+// Now ensure merging gets the expected behavior
+// RUN: %clang -c -flto %s -o %t0.o
+// RUN: %clang -c -flto -fveclib=Accelerate %s -o %t1.o
+// RUN: %clang -c -flto -fveclib=SVML %s -o %t2.o
+// Merge none with -fveclib=Accelerate -> VectorLibrary=Accelerate
+// RUN: llvm-lto %t0.o %t1.o -o %t3.o -save-merged-module
+// RUN: llvm-dis %t3.o.merged.bc -o - | FileCheck %s --check-prefix=ACCELERATE
+// Merge none with -fveclib=SVML -> VectorLibrary=SVML
+// RUN: llvm-lto %t0.o %t2.o -o %t3.o -save-merged-module
+// RUN: llvm-dis %t3.o.merged.bc -o - | FileCheck %s --check-prefix=SVML
+// Merge -fveclib=Accelerate with -fveclib=SVML -> Error
+// RUN: not llvm-lto %t1.o %t2.o -o %t3.o -save-merged-module

llvm/lib/LTO/LTOBackend.cpp

MPM = PB.buildThinLTODefaultPipeline(OL, Conf.DebugPassManager,
                                          ImportSummary);
   else
     MPM = PB.buildLTODefaultPipeline(OL, Conf.DebugPassManager, ExportSummary);
   MPM.run(Mod, MAM);
 
   // FIXME (davide): verify the output.
 }
 
+static TargetLibraryInfoImpl *createTLII(Module &Mod, TargetMachine *TM) {
+  TargetLibraryInfoImpl *TLII =
+      new TargetLibraryInfoImpl(Triple(TM->getTargetTriple()));
+  if (auto *MD = mdconst::extract_or_null<ConstantInt>(
+          Mod.getModuleFlag("DisableAllBuiltins"))) {
+    if (MD->getZExtValue())
+      TLII->disableAllFunctions();
+  } else if (Metadata *Val = Mod.getModuleFlag("NoBuiltins")) {
+    // Disable individual libc/libm calls in TargetLibraryInfo.
+    LibFunc F;
+    for (const MDOperand &FuncName : cast<MDNode>(Val)->operands())
+      if (TLII->getLibFunc(cast<MDString>(*FuncName).getString(), F))
+        TLII->setUnavailable(F);
+  }
+
+  if (MDString *VL =
+          dyn_cast_or_null<MDString>(Mod.getModuleFlag("VectorLibrary"))) {
+    if (VL->getString() == "Accelerate")
+      TLII->addVectorizableFunctionsFromVecLib(
+          TargetLibraryInfoImpl::Accelerate);
+    else if (VL->getString() == "SVML")
+      TLII->addVectorizableFunctionsFromVecLib(TargetLibraryInfoImpl::SVML);
+    else
+      llvm_unreachable("Invalid vector library module flag");
+  }
+  return TLII;
+}
+
 static void runNewPMCustomPasses(Module &Mod, TargetMachine *TM,
                                  std::string PipelineDesc,
                                  std::string AAPipelineDesc,
                                  bool DisableVerify) {
   PassBuilder PB(TM);
   AAManager AA;
 
   // Parse a custom AA pipeline if asked to.
   if (!AAPipelineDesc.empty())
     if (auto Err = PB.parseAAPipeline(AA, AAPipelineDesc))
       report_fatal_error("unable to parse AA pipeline description '" +
                          AAPipelineDesc + "': " + toString(std::move(Err)));
 
   LoopAnalysisManager LAM;
   FunctionAnalysisManager FAM;
   CGSCCAnalysisManager CGAM;
   ModuleAnalysisManager MAM;
 
   // Register the AA manager first so that our version is the one used.
   FAM.registerPass([&] { return std::move(AA); });
 
+  std::unique_ptr<TargetLibraryInfoImpl> TLII(createTLII(Mod, TM));
+  FAM.registerPass([&] { return TargetLibraryAnalysis(*TLII); });
+  MAM.registerPass([&] { return TargetLibraryAnalysis(*TLII); });
+
   // Register all the basic analyses with the managers.
   PB.registerModuleAnalyses(MAM);
   PB.registerCGSCCAnalyses(CGAM);
   PB.registerFunctionAnalyses(FAM);
   PB.registerLoopAnalyses(LAM);
   PB.crossRegisterProxies(LAM, FAM, CGAM, MAM);
 
   ModulePassManager MPM;

 
 static void runOldPMPasses(Config &Conf, Module &Mod, TargetMachine *TM,
                            bool IsThinLTO, ModuleSummaryIndex *ExportSummary,
                            const ModuleSummaryIndex *ImportSummary) {
   legacy::PassManager passes;
   passes.add(createTargetTransformInfoWrapperPass(TM->getTargetIRAnalysis()));
 
   PassManagerBuilder PMB;
-  PMB.LibraryInfo = new TargetLibraryInfoImpl(Triple(TM->getTargetTriple()));
+  PMB.LibraryInfo = createTLII(Mod, TM);
   PMB.Inliner = createFunctionInliningPass();
   PMB.ExportSummary = ExportSummary;
   PMB.ImportSummary = ImportSummary;
   // Unconditionally verify input since it is not verified before this
   // point and has unknown origin.
   PMB.VerifyInput = true;
   PMB.VerifyOutput = !Conf.DisableVerify;
   PMB.LoopVectorize = true;