Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FMV][GlobalOpt] Add an option for static resolution of non-FMV callers. #123757

Merged

Conversation

labrinea
Copy link
Collaborator

Adds optimize-non-fmv-callers (disabled by default) as a short term workaround to keep the llvm testsuite bots green, until we decide what is the right solution for the problem which was previously addressed with #123383.

Adds `optimize-non-fmv-callers` (disabled by default) as a short term
workaround to keep the llvm testsuite bots green, until we decide what
is the right solution for the problem which was previously addressed
with llvm#123383.
@llvmbot
Copy link
Member

llvmbot commented Jan 21, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Alexandros Lamprineas (labrinea)

Changes

Adds optimize-non-fmv-callers (disabled by default) as a short term workaround to keep the llvm testsuite bots green, until we decide what is the right solution for the problem which was previously addressed with #123383.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/GlobalOpt.cpp (+24-11)
diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index eb97d8b4a74f35..00c20ad5f37094 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -95,6 +95,21 @@ STATISTIC(NumIFuncsDeleted, "Number of IFuncs removed");
 STATISTIC(NumGlobalArraysPadded,
           "Number of global arrays padded to alignment boundary");
 
+// FIXME:
+// Optimizing non-FMV callers is causing a regression in the llvm test suite,
+// specifically a 'predres' version is unexpectedly trapping on GravitonG4.
+// My explanation is that when the caller in not a versioned function, the
+// compiler exclusively relies on the command line option, or target attribute
+// to deduce whether a feature is available. However, there is no guarantee
+// that in reality the host supports those implied features, which arguably
+// is a user error. This option allows disabling the optimization as a short
+// term workaround to keep the bots green.
+static cl::opt<bool>
+    OptimizeNonFMVCallers("optimize-non-fmv-callers",
+                          cl::desc("Statically resolve calls to versioned "
+                                   "functions from non-versioned callers."),
+                          cl::init(false), cl::Hidden);
+
 static cl::opt<bool>
     EnableColdCCStressTest("enable-coldcc-stress-test",
                            cl::desc("Enable stress test of coldcc by adding "
@@ -2715,6 +2730,9 @@ static bool OptimizeNonTrivialIFuncs(
 
     assert(!Callees.empty() && "Expecting successful collection of versions");
 
+    LLVM_DEBUG(dbgs() << "Statically resolving calls to function "
+                      << Resolver->getName() << "\n");
+
     // Cache the feature mask for each callee.
     for (Function *Callee : Callees) {
       auto [It, Inserted] = FeatureMask.try_emplace(Callee);
@@ -2785,20 +2803,15 @@ static bool OptimizeNonTrivialIFuncs(
       } else {
         // We can't reason much about non-FMV callers. Just pick the highest
         // priority callee if it matches, otherwise bail.
-        // if (I > 0 || !implies(CallerBits, CalleeBits))
-        //
-        // FIXME: This is causing a regression in the llvm test suite,
-        // specifically a 'predres' version is unexpectedly trapping on
-        // GravitonG4. My explanation is that when the caller in not a
-        // versioned function, the compiler exclusively relies on the
-        // command line option, or target attribute to deduce whether a
-        // feature is available. However, there is no guarantee that in
-        // reality the host supports those implied features.
-        continue;
+        if (!OptimizeNonFMVCallers || I > 0 || !implies(CallerBits, CalleeBits))
+          continue;
       }
       auto &Calls = CallSites[Caller];
-      for (CallBase *CS : Calls)
+      for (CallBase *CS : Calls) {
+        LLVM_DEBUG(dbgs() << "Redirecting call " << Caller->getName() << " -> "
+                          << Callee->getName() << "\n");
         CS->setCalledOperand(Callee);
+      }
       Changed = true;
     }
     if (IF.use_empty() ||

@labrinea labrinea requested a review from jroelofs January 21, 2025 14:41
@labrinea labrinea merged commit cf6d79a into llvm:main Jan 21, 2025
10 checks passed
@labrinea labrinea deleted the option-for-static-resolution-of-non-fmv-callers branch January 21, 2025 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants