[FuncComp] Compare MDNodes in cmpMetadata using cmpMDNode. #128878
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-llvm-transforms Author: Florian Hahn (fhahn) ChangesUse cmpMDNode in cmpMetadata to structurally compare MDNodes for metadata arguments. This fixes a mis-compile caused by cmpMetadata incorrectly returning 0 for different nodes. Note that metadata can contain cycles, so we need to make sure we don't get stuck in an infinite cycle. Full diff: https://github.com/llvm/llvm-project/pull/128878.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Transforms/Utils/FunctionComparator.h b/llvm/include/llvm/Transforms/Utils/FunctionComparator.h
index 19c5f7449f23e..47871719b497a 100644
--- a/llvm/include/llvm/Transforms/Utils/FunctionComparator.h
+++ b/llvm/include/llvm/Transforms/Utils/FunctionComparator.h
@@ -328,8 +328,12 @@ class FunctionComparator {
int cmpOrderings(AtomicOrdering L, AtomicOrdering R) const;
int cmpInlineAsm(const InlineAsm *L, const InlineAsm *R) const;
int cmpAttrs(const AttributeList L, const AttributeList R) const;
- int cmpMDNode(const MDNode *L, const MDNode *R) const;
- int cmpMetadata(const Metadata *L, const Metadata *R) const;
+ int cmpMDNode(const MDNode *L, const MDNode *R,
+ SmallPtrSetImpl<const MDNode *> &SeenL,
+ SmallPtrSetImpl<const MDNode *> &SeenR) const;
+ int cmpMetadata(const Metadata *L, const Metadata *R,
+ SmallPtrSetImpl<const MDNode *> &SeenL,
+ SmallPtrSetImpl<const MDNode *> &SeenR) const;
int cmpInstMetadata(Instruction const *L, Instruction const *R) const;
int cmpOperandBundlesSchema(const CallBase &LCS, const CallBase &RCS) const;
diff --git a/llvm/lib/Transforms/Utils/FunctionComparator.cpp b/llvm/lib/Transforms/Utils/FunctionComparator.cpp
index 6d4026e8209de..8176518d82011 100644
--- a/llvm/lib/Transforms/Utils/FunctionComparator.cpp
+++ b/llvm/lib/Transforms/Utils/FunctionComparator.cpp
@@ -185,8 +185,13 @@ int FunctionComparator::cmpAttrs(const AttributeList L,
return 0;
}
-int FunctionComparator::cmpMetadata(const Metadata *L,
- const Metadata *R) const {
+int FunctionComparator::cmpMetadata(
+ const Metadata *L, const Metadata *R,
+ SmallPtrSetImpl<const MDNode *> &SeenL,
+ SmallPtrSetImpl<const MDNode *> &SeenR) const {
+ if (L == R)
+ return 0;
+
// TODO: the following routine coerce the metadata contents into constants
// or MDStrings before comparison.
// It ignores any other cases, so that the metadata nodes are considered
@@ -207,22 +212,51 @@ int FunctionComparator::cmpMetadata(const Metadata *L,
auto *CL = dyn_cast<ConstantAsMetadata>(L);
auto *CR = dyn_cast<ConstantAsMetadata>(R);
- if (CL == CR)
- return 0;
- if (!CL)
+ if (CL && CR) {
+ if (!CL)
+ return -1;
+ if (!CR)
+ return 1;
+ return cmpConstants(CL->getValue(), CR->getValue());
+ }
+
+ auto *NodeL = dyn_cast<const MDNode>(L);
+ auto *NodeR = dyn_cast<const MDNode>(R);
+ if (NodeL && NodeR)
+ return cmpMDNode(NodeL, NodeR, SeenL, SeenR);
+
+ if (NodeR)
return -1;
- if (!CR)
+
+ if (NodeL)
return 1;
- return cmpConstants(CL->getValue(), CR->getValue());
+
+ assert(false);
+
+ return 0;
}
-int FunctionComparator::cmpMDNode(const MDNode *L, const MDNode *R) const {
+int FunctionComparator::cmpMDNode(
+ const MDNode *L, const MDNode *R, SmallPtrSetImpl<const MDNode *> &SeenL,
+ SmallPtrSetImpl<const MDNode *> &SeenR) const {
if (L == R)
return 0;
if (!L)
return -1;
if (!R)
return 1;
+
+ // Check if we already checked either L or R previosuly. This can be the case
+ // for metadata nodes with cycles.
+ bool AlreadySeenL = !SeenL.insert(L).second;
+ bool AlreadySeenR = !SeenR.insert(R).second;
+ if (AlreadySeenL && AlreadySeenR)
+ return 0;
+ if (AlreadySeenR)
+ return -1;
+ if (AlreadySeenL)
+ return 1;
+
// TODO: Note that as this is metadata, it is possible to drop and/or merge
// this data when considering functions to merge. Thus this comparison would
// return 0 (i.e. equivalent), but merging would become more complicated
@@ -232,7 +266,7 @@ int FunctionComparator::cmpMDNode(const MDNode *L, const MDNode *R) const {
if (int Res = cmpNumbers(L->getNumOperands(), R->getNumOperands()))
return Res;
for (size_t I = 0; I < L->getNumOperands(); ++I)
- if (int Res = cmpMetadata(L->getOperand(I), R->getOperand(I)))
+ if (int Res = cmpMetadata(L->getOperand(I), R->getOperand(I), SeenL, SeenR))
return Res;
return 0;
}
@@ -254,7 +288,10 @@ int FunctionComparator::cmpInstMetadata(Instruction const *L,
auto const [KeyR, MR] = MDR[I];
if (int Res = cmpNumbers(KeyL, KeyR))
return Res;
- if (int Res = cmpMDNode(ML, MR))
+
+ SmallPtrSet<const MDNode *, 4> SeenL;
+ SmallPtrSet<const MDNode *, 4> SeenR;
+ if (int Res = cmpMDNode(ML, MR, SeenL, SeenR))
return Res;
}
return 0;
@@ -721,8 +758,11 @@ int FunctionComparator::cmpOperations(const Instruction *L,
if (int Res = cmpNumbers(CI->getTailCallKind(),
cast<CallInst>(R)->getTailCallKind()))
return Res;
+
+ SmallPtrSet<const MDNode *, 4> SeenL;
+ SmallPtrSet<const MDNode *, 4> SeenR;
return cmpMDNode(L->getMetadata(LLVMContext::MD_range),
- R->getMetadata(LLVMContext::MD_range));
+ R->getMetadata(LLVMContext::MD_range), SeenL, SeenR);
}
if (const InsertValueInst *IVI = dyn_cast<InsertValueInst>(L)) {
ArrayRef<unsigned> LIndices = IVI->getIndices();
@@ -895,11 +935,10 @@ int FunctionComparator::cmpValues(const Value *L, const Value *R) const {
const MetadataAsValue *MetadataValueL = dyn_cast<MetadataAsValue>(L);
const MetadataAsValue *MetadataValueR = dyn_cast<MetadataAsValue>(R);
if (MetadataValueL && MetadataValueR) {
- if (MetadataValueL == MetadataValueR)
- return 0;
-
+ SmallPtrSet<const MDNode *, 4> SeenL;
+ SmallPtrSet<const MDNode *, 4> SeenR;
return cmpMetadata(MetadataValueL->getMetadata(),
- MetadataValueR->getMetadata());
+ MetadataValueR->getMetadata(), SeenL, SeenR);
}
if (MetadataValueL)
diff --git a/llvm/test/Transforms/MergeFunc/metadata-call-arguments.ll b/llvm/test/Transforms/MergeFunc/metadata-call-arguments.ll
index 28263741f2cde..10b4c691d314a 100644
--- a/llvm/test/Transforms/MergeFunc/metadata-call-arguments.ll
+++ b/llvm/test/Transforms/MergeFunc/metadata-call-arguments.ll
@@ -1,7 +1,6 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --include-generated-funcs --version 5
; RUN: opt -p mergefunc -S %s | FileCheck %s
-; FIXME: Should not be merged with @call_mdtuple_arg_not_equal_2.
define i64 @call_mdtuple_arg_not_equal_1() {
%r = call i64 @llvm.read_volatile_register.i64(metadata !0)
ret i64 %r
@@ -22,7 +21,6 @@ define i64 @call_mdtuple_arg_with_cycle_equal_2() {
ret i64 %r
}
-; FIXME: Should not be merged with @call_mdtuple_arg_with_cycle_not_equal_2.
define i64 @call_mdtuple_arg_with_cycle_not_equal_1() {
%r = call i64 @llvm.read_volatile_register.i64(metadata !3)
ret i64 %r
@@ -55,48 +53,50 @@ declare i64 @llvm.read_volatile_register.i64(metadata)
!5 = !{!"foo", i64 10}
!6 = !{!"foo", i64 10}
+; CHECK-LABEL: define i64 @call_mdtuple_arg_not_equal_1() {
+; CHECK-NEXT: [[TMP1:%.*]] = call i64 @llvm.read_volatile_register.i64(metadata [[META0:![0-9]+]])
+; CHECK-NEXT: ret i64 [[TMP1]]
+;
+;
+; CHECK-LABEL: define i64 @call_mdtuple_arg_not_equal_2() {
+; CHECK-NEXT: [[TMP1:%.*]] = call i64 @llvm.read_volatile_register.i64(metadata [[META1:![0-9]+]])
+; CHECK-NEXT: ret i64 [[TMP1]]
+;
+;
; CHECK-LABEL: define i64 @call_mdtuple_arg_with_cycle_equal_1() {
-; CHECK-NEXT: [[R:%.*]] = call i64 @llvm.read_volatile_register.i64(metadata [[META0:![0-9]+]])
+; CHECK-NEXT: [[R:%.*]] = call i64 @llvm.read_volatile_register.i64(metadata [[META2:![0-9]+]])
; CHECK-NEXT: ret i64 [[R]]
;
;
; CHECK-LABEL: define i64 @call_mdtuple_arg_with_cycle_equal_2() {
-; CHECK-NEXT: [[R:%.*]] = call i64 @llvm.read_volatile_register.i64(metadata [[META1:![0-9]+]])
+; CHECK-NEXT: [[R:%.*]] = call i64 @llvm.read_volatile_register.i64(metadata [[META3:![0-9]+]])
; CHECK-NEXT: ret i64 [[R]]
;
;
; CHECK-LABEL: define i64 @call_mdtuple_arg_with_cycle_not_equal_1() {
-; CHECK-NEXT: [[R:%.*]] = call i64 @llvm.read_volatile_register.i64(metadata [[META1]])
+; CHECK-NEXT: [[R:%.*]] = call i64 @llvm.read_volatile_register.i64(metadata [[META3]])
; CHECK-NEXT: ret i64 [[R]]
;
;
; CHECK-LABEL: define i64 @call_mdtuple_arg_with_cycle_not_equal_2() {
-; CHECK-NEXT: [[R:%.*]] = call i64 @llvm.read_volatile_register.i64(metadata [[META2:![0-9]+]])
+; CHECK-NEXT: [[R:%.*]] = call i64 @llvm.read_volatile_register.i64(metadata [[META4:![0-9]+]])
; CHECK-NEXT: ret i64 [[R]]
;
;
; CHECK-LABEL: define i64 @call_mdtuple_arg_equal_1() {
-; CHECK-NEXT: [[R:%.*]] = call i64 @llvm.read_volatile_register.i64(metadata [[META3:![0-9]+]])
+; CHECK-NEXT: [[R:%.*]] = call i64 @llvm.read_volatile_register.i64(metadata [[META5:![0-9]+]])
; CHECK-NEXT: ret i64 [[R]]
;
;
-; CHECK-LABEL: define i64 @call_mdtuple_arg_not_equal_2() {
-; CHECK-NEXT: [[TMP1:%.*]] = tail call i64 @call_mdtuple_arg_equal_1()
-; CHECK-NEXT: ret i64 [[TMP1]]
-;
-;
-; CHECK-LABEL: define i64 @call_mdtuple_arg_not_equal_1() {
-; CHECK-NEXT: [[TMP1:%.*]] = tail call i64 @call_mdtuple_arg_equal_1()
-; CHECK-NEXT: ret i64 [[TMP1]]
-;
-;
; CHECK-LABEL: define i64 @call_mdtuple_arg_equal_2() {
; CHECK-NEXT: [[TMP1:%.*]] = tail call i64 @call_mdtuple_arg_equal_1()
; CHECK-NEXT: ret i64 [[TMP1]]
;
;.
-; CHECK: [[META0]] = distinct !{[[META0]], !"foo"}
-; CHECK: [[META1]] = distinct !{[[META1]], !"foo"}
-; CHECK: [[META2]] = distinct !{[[META2]], !"bar"}
-; CHECK: [[META3]] = !{!"foo", i64 10}
+; CHECK: [[META0]] = !{!"foo"}
+; CHECK: [[META1]] = !{!"bar"}
+; CHECK: [[META2]] = distinct !{[[META2]], !"foo"}
+; CHECK: [[META3]] = distinct !{[[META3]], !"foo"}
+; CHECK: [[META4]] = distinct !{[[META4]], !"bar"}
+; CHECK: [[META5]] = !{!"foo", i64 10}
;.
|
nikic
reviewed
Feb 26, 2025
Contributor
nikic
left a comment
nikic
left a comment
There was a problem hiding this comment.
I'm somewhat unhappy about doing deep metadata comparison. IMHO, we should not be comparing instruction metadata at all, and instead doing a combineMetadata run to combine metadata between the merged functions. This results in more merging opportunities, while also avoiding the need to implement complex comparison logic.
We do need to compare metadata arguments to calls, but I think the inputs that are used there are a lot more limited -- I think it's pretty much only MDStrings?
| if (CL == CR) | ||
| return 0; | ||
| if (!CL) | ||
| if (CL && CR) { |
Contributor
There was a problem hiding this comment.
Was this supposed to be?
Suggested change
| if (CL && CR) { | |
| if (CL || CR) { |
| } | ||
|
|
||
| auto *NodeL = dyn_cast<const MDNode>(L); | ||
| auto *NodeR = dyn_cast<const MDNode>(R); |
Contributor
There was a problem hiding this comment.
Is the const in the dyn_cast needed?
| return 1; | ||
| return cmpConstants(CL->getValue(), CR->getValue()); | ||
|
|
||
| assert(false); |
| if (!R) | ||
| return 1; | ||
|
|
||
| // Check if we already checked either L or R previosuly. This can be the case |
Contributor
There was a problem hiding this comment.
Suggested change
| // Check if we already checked either L or R previosuly. This can be the case | |
| // Check if we already checked either L or R previously. This can be the case |
| bool AlreadySeenL = !SeenL.insert(L).second; | ||
| bool AlreadySeenR = !SeenR.insert(R).second; | ||
| if (AlreadySeenL && AlreadySeenR) | ||
| return 0; |
Contributor
There was a problem hiding this comment.
I don't think that both being recursive really guarantees that they are equal, they may recurse in different ways, no?
| ; CHECK: [[META1]] = !{!"bar"} | ||
| ; CHECK: [[META2]] = distinct !{[[META2]], !"foo"} | ||
| ; CHECK: [[META3]] = distinct !{[[META3]], !"foo"} | ||
| ; CHECK: [[META4]] = distinct !{[[META4]], !"bar"} |
Contributor
There was a problem hiding this comment.
These don't look like valid read_volatile_register arguments? What is your actual motivation here?
Member
There was a problem hiding this comment.
Actual motivation is to fix something like this...
uintptr_t read_vbar() {
return __arm_rsr64("VBAR_EL1");
}
uintptr_t read_ttbr0() {
return __arm_rsr64("TTBR0_EL1");
}
...from miscompiling under -fmerge-functions. See https://godbolt.org/z/qf6zWqzP7.
fhahn
commented
Mar 11, 2025
Contributor
Author
fhahn
left a comment
fhahn
left a comment
There was a problem hiding this comment.
I'm somewhat unhappy about doing deep metadata comparison. IMHO, we should not be comparing instruction metadata at all, and instead doing a combineMetadata run to combine metadata between the merged functions. This results in more merging opportunities, while also avoiding the need to implement complex comparison logic.
We do need to compare metadata arguments to calls, but I think the inputs that are used there are a lot more limited -- I think it's pretty much only MDStrings?
There are a few intrinsics that take more complex metadata args including llvm.experimental.noalias.scope.decl(which has alias scope sets which may have self-references and llvm.type.test). I'll see if it would be possible to impose a simple set of restrictions on metadata as arguments.
fhahn
force-pushed
the
func-comp-mdnodes
branch
from
87baa8f to
df6a046
Compare
fhahn
commented
Mar 22, 2025
Contributor
Author
fhahn
left a comment
fhahn
left a comment
There was a problem hiding this comment.
I updated the PR to restrict the types of metadata nodes allowed in metadata arguments (most important there's guaranteed that there are no cycles). This should allow all current uses, should we document the restrictions?
This should allow recursing in FunctionComperator in contexts where the metadata as function argument.
fhahn
force-pushed
the
func-comp-mdnodes
branch
from
df6a046 to
3ad4ccc
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use cmpMDNode in cmpMetadata to structurally compare MDNodes for metadata arguments. This fixes a mis-compile caused by cmpMetadata incorrectly returning 0 for different nodes.
Note that metadata can contain cycles, so we need to make sure we don't get stuck in an infinite cycle.