PlaceSafepoints: Fix using default constructed TargetLibraryInfo

[arsenm]

PlaceSafepoints has an awful hack where it's creating a legacy
PassManager inside it's runImpl, which was not propagating the incoming
TLI. This means there's an implicit bug fix, where PlaceSafepoints would have
been treating too many calls as builtins.

Split from #92400

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Matt Arsenault (arsenm)

Changes

PlaceSafepoints has an awful hack where it's creating a legacy
PassManager inside it's runImpl, which was not propagating the incoming
TLI. This means there's an implicit bug fix, where PlaceSafepoints would have
been treating too many calls as builtins.

Split from #92400

---

Full diff: https://github.com/llvm/llvm-project/pull/92411.diff

4 Files Affected:

• (modified) llvm/include/llvm/Analysis/TargetLibraryInfo.h (+4)
• (modified) llvm/lib/Analysis/TargetLibraryInfo.cpp (+4)
• (modified) llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp (+2)
• (modified) llvm/test/Transforms/PlaceSafepoints/libcall.ll (+5-2)

diff --git a/llvm/include/llvm/Analysis/TargetLibraryInfo.h b/llvm/include/llvm/Analysis/TargetLibraryInfo.h
index 46f31f918e7b6..f5da222d11f55 100644
--- a/llvm/include/llvm/Analysis/TargetLibraryInfo.h
+++ b/llvm/include/llvm/Analysis/TargetLibraryInfo.h
@@ -633,6 +633,10 @@ class TargetLibraryInfoWrapperPass : public ImmutablePass {
   explicit TargetLibraryInfoWrapperPass(const Triple &T);
   explicit TargetLibraryInfoWrapperPass(const TargetLibraryInfoImpl &TLI);
 
+  // FIXME: This should be removed when PlaceSafepoints is fixed to not create a
+  // PassManager inside a pass.
+  explicit TargetLibraryInfoWrapperPass(const TargetLibraryInfo &TLI);
+
   TargetLibraryInfo &getTLI(const Function &F) {
     FunctionAnalysisManager DummyFAM;
     TLI = TLA.run(F, DummyFAM);
diff --git a/llvm/lib/Analysis/TargetLibraryInfo.cpp b/llvm/lib/Analysis/TargetLibraryInfo.cpp
index c62d9daa13ef0..684b0759f3157 100644
--- a/llvm/lib/Analysis/TargetLibraryInfo.cpp
+++ b/llvm/lib/Analysis/TargetLibraryInfo.cpp
@@ -1383,6 +1383,10 @@ TargetLibraryInfoWrapperPass::TargetLibraryInfoWrapperPass(
   initializeTargetLibraryInfoWrapperPassPass(*PassRegistry::getPassRegistry());
 }
 
+TargetLibraryInfoWrapperPass::TargetLibraryInfoWrapperPass(
+    const TargetLibraryInfo &TLIOther)
+    : TargetLibraryInfoWrapperPass(*TLIOther.Impl) {}
+
 AnalysisKey TargetLibraryAnalysis::Key;
 
 // Register the basic pass.
diff --git a/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp b/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp
index 77d155d7e78e3..dcdea6e7b62ae 100644
--- a/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp
+++ b/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp
@@ -288,6 +288,8 @@ bool PlaceSafepointsPass::runImpl(Function &F, const TargetLibraryInfo &TLI) {
     // with for the moment.
     legacy::FunctionPassManager FPM(F.getParent());
     bool CanAssumeCallSafepoints = enableCallSafepoints(F);
+
+    FPM.add(new TargetLibraryInfoWrapperPass(TLI));
     auto *PBS = new PlaceBackedgeSafepointsLegacyPass(CanAssumeCallSafepoints);
     FPM.add(PBS);
     FPM.run(F);
diff --git a/llvm/test/Transforms/PlaceSafepoints/libcall.ll b/llvm/test/Transforms/PlaceSafepoints/libcall.ll
index 89090ba1b243c..6e26f924a5ba3 100644
--- a/llvm/test/Transforms/PlaceSafepoints/libcall.ll
+++ b/llvm/test/Transforms/PlaceSafepoints/libcall.ll
@@ -1,4 +1,6 @@
-; RUN: opt -S -passes=place-safepoints < %s | FileCheck %s
+; RUN: opt -S -passes=place-safepoints < %s | FileCheck -check-prefixes=CHECK,WITHLDEXPF %s
+; RUN: opt -S -passes=place-safepoints -disable-builtin=ldexp < %s | FileCheck %s
+
 
 ; Libcalls will not contain a safepoint poll, so check that we insert
 ; a safepoint in a loop containing a libcall.
@@ -17,7 +19,8 @@ loop:
 ; CHECK-NEXT: %x_loop = phi double [ %x, %entry ], [ %x_exp, %loop ]
 ; CHECK-NEXT: %x_exp = call double @ldexp(double %x_loop, i32 5)
 ; CHECK-NEXT: %done = fcmp ogt double %x_exp, 1.5
-; CHECK-NEXT: call void @do_safepoint
+; WITHLDEXPF-NEXT: call void @do_safepoint
+; CHECK-NEXT: br
   %x_loop = phi double [ %x, %entry ], [ %x_exp, %loop ]
   %x_exp = call double @ldexp(double %x_loop, i32 5) nounwind readnone
   %done = fcmp ogt double %x_exp, 1.5

[preames]

LGTM so that you can make forward progress.

We really should just delete PlaceSafepoints. To my knowledge, no one is using this.

(Edit for correction which follows)

Checking the commit history more carefully, I'm wrong about there being no recent usage. However, the use of the legacy pass manager inside a pass which has been ported to the new pass manager otherwise is a very serious code smell.