[FunctionComparator] Differentiate instructions passing different MDStrings

[NuriAmari]

Prior to this patch, differing metadata operands to two otherwise identical instructions was not enough to consider the instructions different in the eyes of the function comparator. This breaks LLVM virtual function elimination, among other features.

In this patch, we handle the case where two associated operands are MDStrings of different value. This patch does not differentiate more complex metadata operands.

[NuriAmari]

There are a couple related patches: #65963, 8325d46, but they don't solve this problem.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Nuri Amari (NuriAmari)

Changes

Prior to this patch, differing metadata operands to two otherwise identical instructions was not enough to consider the instructions distinct in the eyes of the function comparator. This breaks LLVM virtual function elimination, among other features.

In this patch, we handle the case where two associated operands are MDStrings of different value. This patch does not differentiate more complex metadata operands.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/FunctionComparator.cpp (+29-1)
• (added) llvm/test/Transforms/MergeFunc/mergefunc-preserve-vfe-intrinsics.ll (+34)

diff --git a/llvm/lib/Transforms/Utils/FunctionComparator.cpp b/llvm/lib/Transforms/Utils/FunctionComparator.cpp
index b1d74f67377e27f..79ca99d1566ce25 100644
--- a/llvm/lib/Transforms/Utils/FunctionComparator.cpp
+++ b/llvm/lib/Transforms/Utils/FunctionComparator.cpp
@@ -160,10 +160,23 @@ int FunctionComparator::cmpAttrs(const AttributeList L,
 int FunctionComparator::cmpMetadata(const Metadata *L,
                                     const Metadata *R) const {
   // TODO: the following routine coerce the metadata contents into constants
-  // before comparison.
+  // or MDStrings before comparison.
   // It ignores any other cases, so that the metadata nodes are considered
   // equal even though this is not correct.
   // We should structurally compare the metadata nodes to be perfect here.
+
+  auto *MDStringL = dyn_cast<MDString>(L);
+  auto *MDStringR = dyn_cast<MDString>(R);
+  if (MDStringL && MDStringR) {
+    if (MDStringL == MDStringR)
+      return 0;
+    return MDStringL->getString().compare(MDStringR->getString());
+  }
+  if (MDStringR)
+    return -1;
+  if (MDStringL)
+    return 1;
+
   auto *CL = dyn_cast<ConstantAsMetadata>(L);
   auto *CR = dyn_cast<ConstantAsMetadata>(R);
   if (CL == CR)
@@ -820,6 +833,21 @@ int FunctionComparator::cmpValues(const Value *L, const Value *R) const {
   if (ConstR)
     return -1;
 
+  const MetadataAsValue *MetadataValueL = dyn_cast<MetadataAsValue>(L);
+  const MetadataAsValue *MetadataValueR = dyn_cast<MetadataAsValue>(R);
+  if (MetadataValueL && MetadataValueR) {
+    if (MetadataValueL == MetadataValueR)
+      return 0;
+
+    return cmpMetadata(MetadataValueL->getMetadata(),
+                       MetadataValueR->getMetadata());
+  }
+
+  if (MetadataValueL)
+    return 1;
+  if (MetadataValueR)
+    return -1;
+
   const InlineAsm *InlineAsmL = dyn_cast<InlineAsm>(L);
   const InlineAsm *InlineAsmR = dyn_cast<InlineAsm>(R);
 
diff --git a/llvm/test/Transforms/MergeFunc/mergefunc-preserve-vfe-intrinsics.ll b/llvm/test/Transforms/MergeFunc/mergefunc-preserve-vfe-intrinsics.ll
new file mode 100644
index 000000000000000..89fa6f82469610d
--- /dev/null
+++ b/llvm/test/Transforms/MergeFunc/mergefunc-preserve-vfe-intrinsics.ll
@@ -0,0 +1,34 @@
+; This test contains three identical functions, aside from the metadata
+; they pass to a function call. This test verifies that the function merger
+; pass is able to merge the two functions that are truly identifical,
+; but the third that passes different metadata is preserved
+
+; RUN: opt -passes=mergefunc -S %s | FileCheck %s
+
+declare { ptr, i1 } @llvm.type.checked.load(ptr, i32, metadata)
+
+define i1 @merge_candidate_a(ptr %ptr, i32 %offset) {
+    %1 = call { ptr, i1 } @llvm.type.checked.load(ptr %ptr, i32 %offset, metadata !"common_metadata")
+    %2 = extractvalue { ptr, i1 } %1, 1
+    ret i1 %2
+}
+
+define i1 @merge_candidate_c(ptr %ptr, i32 %offset) {
+    %1 = call { ptr, i1 } @llvm.type.checked.load(ptr %ptr, i32 %offset, metadata !"different_metadata")
+    %2 = extractvalue { ptr, i1 } %1, 1
+    ret i1 %2
+}
+; CHECK-LABEL: @merge_candidate_c
+; CHECK-NOT: call i1 merge_candidate_a
+; CHECK: call { ptr, i1 } @llvm.type.checked.load
+; CHECK-NOT: call i1 merge_candidate_a
+; CHECK: ret
+
+define i1 @merge_candidate_b(ptr %ptr, i32 %offset) {
+    %1 = call { ptr, i1 } @llvm.type.checked.load(ptr %ptr, i32 %offset, metadata !"common_metadata")
+    %2 = extractvalue { ptr, i1 } %1, 1
+    ret i1 %2
+}
+; CHECK-LABEL: @merge_candidate_b
+; CHECK: call i1 @merge_candidate_a
+; CHECK: ret

[nikic]

LGTM

[dexonsmith]

Can you change the subject of the commit / pull request before pushing to avoid using the phrase "distinct MDString"? I suggest, "different" instead of "distinct". The problem is that "distinct" means something specific in metadata: non-uniqued. Although this cannot be applied to MDString, it's still a bit confusing.

Oops! I misread the cmpMetadata logic

[dexonsmith]

LGTM, if you rephrase the subject of the commit and pull request to use "different" instead of "distinct" when referring to MDString (because of the language clash against distinct vs. uniqued in LLVM metadata).

[NuriAmari]

Thanks for the reviews all!