Refactored llvm::UpgradeIntrinsicCall() function in lib/IR/AutoUpgrade.cpp with lookup table based on #30382

[aliraeisdanaei]

Changed the logic to a lookup table instead of series of if else statements because of #30382

I would appreciate your feedback.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

Author: Ali Raeisdanaei (aliraeisdanaei)

Changes

Changed the logic to a lookup table instead of series of if else statements because of #30382

I would appreciate your feedback.

---

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

1 Files Affected:

• (modified) llvm/lib/IR/AutoUpgrade.cpp (+47-33)

diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp
index 57072715366c9..06db5c024fc8b 100644
--- a/llvm/lib/IR/AutoUpgrade.cpp
+++ b/llvm/lib/IR/AutoUpgrade.cpp
@@ -48,9 +48,13 @@
 #include "llvm/TargetParser/Triple.h"
 #include <cstring>
 #include <numeric>
+#include <unordered_map>
 
 using namespace llvm;
 
+// Type Alias for UpgradeFunctions
+typedef Value *(*UpgradeFunc)(StringRef, CallBase *, Function *, IRBuilder<> &);
+
 static cl::opt<bool>
     DisableAutoUpgradeDebugInfo("disable-auto-upgrade-debug-info",
                                 cl::desc("Disable autoupgrade of debug info"));
@@ -4348,41 +4352,51 @@ void llvm::UpgradeIntrinsicCall(CallBase *CI, Function *NewFn) {
     assert(Name.starts_with("llvm.") && "Intrinsic doesn't start with 'llvm.'");
     Name = Name.substr(5);
 
-    bool IsX86 = Name.consume_front("x86.");
-    bool IsNVVM = Name.consume_front("nvvm.");
-    bool IsAArch64 = Name.consume_front("aarch64.");
-    bool IsARM = Name.consume_front("arm.");
-    bool IsAMDGCN = Name.consume_front("amdgcn.");
-    bool IsDbg = Name.consume_front("dbg.");
-    Value *Rep = nullptr;
+    const std::string prefix_x86 = "x86.";
+    const std::string prefix_nvvm = "nvvm.";
+    const std::string prefix_aarch64 = "aarch64.";
+    const std::string prefix_arm = "arm.";
+    const std::string prefix_amdgcn = "amdgcn.";
+    const std::string prefix_dbg = "dbg.";
+
+    std::unordered_map<StringRef, UpgradeFunc> lookupTable;
+    lookupTable[prefix_x86] = upgradeX86IntrinsicCall;
+    lookupTable[prefix_nvvm] = upgradeNVVMIntrinsicCall;
+    lookupTable[prefix_aarch64] = upgradeAArch64IntrinsicCall;
+    lookupTable[prefix_arm] = upgradeARMIntrinsicCall;
+    lookupTable[prefix_amdgcn] = upgradeAMDGCNIntrinsicCall;
+    lookupTable[prefix_dbg] = upgradeIntrinsicFunction1;
 
-    if (!IsX86 && Name == "stackprotectorcheck") {
-      Rep = nullptr;
-    } else if (IsNVVM) {
-      Rep = upgradeNVVMIntrinsicCall(Name, CI, F, Builder);
-    } else if (IsX86) {
-      Rep = upgradeX86IntrinsicCall(Name, CI, F, Builder);
-    } else if (IsAArch64) {
-      Rep = upgradeAArch64IntrinsicCall(Name, CI, F, Builder);
-    } else if (IsARM) {
-      Rep = upgradeARMIntrinsicCall(Name, CI, F, Builder);
-    } else if (IsAMDGCN) {
-      Rep = upgradeAMDGCNIntrinsicCall(Name, CI, F, Builder);
-    } 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);
+    Value *Rep = nullptr;
+    bool not_consumed = true;
+
+    for (auto &entry : lookupTable) {
+      if (Name.consume_front(entry.first)) {
+        not_consumed = false;
+        if (entry.first == prefix_x86 || Name != "stackprotectorcheck") {
+          if (entry.first == prefix_dbg) {
+            // 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 {
+            Rep = entry.second(Name, CI, F, Builder);
+          }
+        }
+        break;
       }
-    } else {
+    }
+
+    if (not_consumed) {
       llvm_unreachable("Unknown function for CallBase upgrade.");
     }
 

[github-actions]

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

[aliraeisdanaei]

Hello @xgupta I would appreciate your feedback on this.

[nikic]

This change makes very little sense. You're creating the table for every single upgrade and then walking over the table. It makes the code slower and does not make it clearer.

[aliraeisdanaei]

This change makes very little sense. You're creating the table for every single upgrade and then walking over the table. It makes the code slower and does not make it clearer.

What do you propose be done? The issue related to not using if else statements. You can initialise a lookup table with all the values, but I don't see how you can avoid the walking over. I'm merely asking for your help. Let me know your ideas.

[nikic]

What do you propose be done?

I don't believe anything needs to be done.

[aliraeisdanaei]

What do you propose be done?

I don't believe anything needs to be done.

Ok fair enough. Why don't we close this PR and Issue #30382 (Edit already closed--I will close this PR)