[RemoveDIs][BC] Reject intrinsic->record upgrades for old-format modules

[SLTozer]

Fixes issue noted at: #86274

When loading bitcode lazily, we may request debug intrinsics be upgraded to debug records during the module parsing phase; later on we perform this upgrade when materializing the module functions. If we change the module's debug info format between parsing and materializing however, then the requested upgrade is no longer correct and leads to an assertion. This patch fixes the issue by adding an extra check in the autoupgrader to see if the upgrade is no longer suitable, and either exit-out or fall back to the correct intrinsic->intrinsic upgrade if one is required.

Adding this check in the autoupgrader is technically not optimal, since we could run the check a single time in a higher up call, but there's no way (that I can see) to do so without leaking through abstractions and making the bitcode/linking/materializing steps more complicated and brittle, and the check isn't particularly expensive.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[OCHyams]

LGTM with a couple of nits.

It's slightly unfortunate that this needs to be done, but I agree this approach makes sense. And eventually it will get ripped out, along with debug intrinsics.

[OCHyams]

nit: Can we add a short comment explaining this variable?

Also, possibly worth inverting it to "FallthroughToDefaultUpgrade" or something, which is slightly more descriptive. Not 100% sure on that, but if you can think of a way to make it slightly more descriptive that would be good. YMMV, this is ok if not.

[OCHyams]

nit: I think this could be slightly more clear, e.g. "We might have decided we don't want the new format after all since requesting the upgrade" -> "We might have decided we don't want the new format after all between requesting the upgrade initially and now;"

(I think "since" is just a slightly ambiguous word used here)

[llvmbot]

@llvm/pr-subscribers-llvm-ir

Author: Stephen Tozer (SLTozer)

Changes

Fixes issue noted at: #86274

When loading bitcode lazily, we may request debug intrinsics be upgraded to debug records during the module parsing phase; later on we perform this upgrade when materializing the module functions. If we change the module's debug info format between parsing and materializing however, then the requested upgrade is no longer correct and leads to an assertion. This patch fixes the issue by adding an extra check in the autoupgrader to see if the upgrade is no longer suitable, and either exit-out or fall back to the correct intrinsic->intrinsic upgrade if one is required.

Adding this check in the autoupgrader is technically not optimal, since we could run the check a single time in a higher up call, but there's no way (that I can see) to do so without leaking through abstractions and making the bitcode/linking/materializing steps more complicated and brittle, and the check isn't particularly expensive.

---

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

2 Files Affected:

• (modified) llvm/include/llvm/IR/AutoUpgrade.h (+2-1)
• (modified) llvm/lib/IR/AutoUpgrade.cpp (+29-10)

diff --git a/llvm/include/llvm/IR/AutoUpgrade.h b/llvm/include/llvm/IR/AutoUpgrade.h
index 152f781ffa9b30..97c3e4d7589d7b 100644
--- a/llvm/include/llvm/IR/AutoUpgrade.h
+++ b/llvm/include/llvm/IR/AutoUpgrade.h
@@ -36,7 +36,8 @@ namespace llvm {
   /// for upgrading, and returns true if it requires upgrading. It may return
   /// null in NewFn if the all calls to the original intrinsic function
   /// should be transformed to non-function-call instructions.
-  bool UpgradeIntrinsicFunction(Function *F, Function *&NewFn);
+  bool UpgradeIntrinsicFunction(Function *F, Function *&NewFn,
+                                bool CanUpgradeDebugIntrinsicsToRecords = true);
 
   /// This is the complement to the above, replacing a specific call to an
   /// intrinsic function with a call to the specified new function.
diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp
index a44f6af4162f03..0f8c984d5e3c7d 100644
--- a/llvm/lib/IR/AutoUpgrade.cpp
+++ b/llvm/lib/IR/AutoUpgrade.cpp
@@ -983,7 +983,8 @@ static Intrinsic::ID shouldUpgradeNVPTXBF16Intrinsic(StringRef Name) {
   return Intrinsic::not_intrinsic;
 }
 
-static bool upgradeIntrinsicFunction1(Function *F, Function *&NewFn) {
+static bool upgradeIntrinsicFunction1(Function *F, Function *&NewFn,
+                                      bool CanUpgradeDebugIntrinsicsToRecords) {
   assert(F && "Illegal to upgrade a non-existent Function.");
 
   StringRef Name = F->getName();
@@ -1057,7 +1058,8 @@ static bool upgradeIntrinsicFunction1(Function *F, Function *&NewFn) {
   case 'd':
     if (Name.consume_front("dbg.")) {
       // Mark debug intrinsics for upgrade to new debug format.
-      if (F->getParent()->IsNewDbgInfoFormat) {
+      if (CanUpgradeDebugIntrinsicsToRecords &&
+          F->getParent()->IsNewDbgInfoFormat) {
         if (Name == "addr" || Name == "value" || Name == "assign" ||
             Name == "declare" || Name == "label") {
           // There's no function to replace these with.
@@ -1413,9 +1415,11 @@ static bool upgradeIntrinsicFunction1(Function *F, Function *&NewFn) {
   return false;
 }
 
-bool llvm::UpgradeIntrinsicFunction(Function *F, Function *&NewFn) {
+bool llvm::UpgradeIntrinsicFunction(Function *F, Function *&NewFn,
+                                    bool CanUpgradeDebugIntrinsicsToRecords) {
   NewFn = nullptr;
-  bool Upgraded = upgradeIntrinsicFunction1(F, NewFn);
+  bool Upgraded =
+      upgradeIntrinsicFunction1(F, NewFn, CanUpgradeDebugIntrinsicsToRecords);
   assert(F != NewFn && "Intrinsic function upgraded to the same function");
 
   // Upgrade intrinsic attributes.  This does not change the function.
@@ -2412,6 +2416,7 @@ void llvm::UpgradeIntrinsicCall(CallBase *CI, Function *NewFn) {
   Builder.SetInsertPoint(CI->getParent(), CI->getIterator());
 
   if (!NewFn) {
+    bool FallthroughToDefaultUpgrade = false;
     // Get the Function's name.
     StringRef Name = F->getName();
 
@@ -4262,16 +4267,30 @@ void llvm::UpgradeIntrinsicCall(CallBase *CI, Function *NewFn) {
       Rep = upgradeARMIntrinsicCall(Name, CI, F, Builder);
     } else if (IsAMDGCN) {
       Rep = upgradeAMDGCNIntrinsicCall(Name, CI, F, Builder);
-    } else if (IsDbg && CI->getModule()->IsNewDbgInfoFormat) {
-      upgradeDbgIntrinsicToDbgRecord(Name, CI);
+    } else if (IsDbg) {
+      // We might have decided we don't want the new format after all between
+      // first requesting the upgrade and now; skip the conversion if that is
+      // the case, and check here to see if the intrinsic needs to be upgraded
+      // normally.
+      if (!CI->getModule()->IsNewDbgInfoFormat) {
+        bool NeedsUpgrade =
+            upgradeIntrinsicFunction1(CI->getCalledFunction(), NewFn, false);
+        if (!NeedsUpgrade)
+          return;
+        FallthroughToDefaultUpgrade = true;
+      } else {
+        upgradeDbgIntrinsicToDbgRecord(Name, CI);
+      }
     } else {
       llvm_unreachable("Unknown function for CallBase upgrade.");
     }
 
-    if (Rep)
-      CI->replaceAllUsesWith(Rep);
-    CI->eraseFromParent();
-    return;
+    if (!FallthroughToDefaultUpgrade) {
+      if (Rep)
+        CI->replaceAllUsesWith(Rep);
+      CI->eraseFromParent();
+      return;
+    }
   }
 
   const auto &DefaultCase = [&]() -> void {

[OCHyams]

(Still LGTM, thanks)