From 4cf46aeae8bd848e397c27d418e60fd25e3d0eae Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 16 Feb 2024 15:34:33 +0100 Subject: [PATCH 1/5] Prevent incorrect constant folding of binary operations involving handle and integer --- src/coreclr/jit/valuenum.cpp | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 977e984865aac7..c2f8c1a3a97c18 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4422,12 +4422,22 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case GT_ROL: case GT_ROR: - case GT_EQ: - case GT_NE: case GT_GT: case GT_GE: case GT_LT: case GT_LE: + if (IsVNHandle(arg0VN) || IsVNHandle(arg1VN)) + { + return false; + } + break; + + case GT_EQ: + case GT_NE: + if (IsVNHandle(arg0VN) != IsVNHandle(arg1VN)) + { + return false; + } break; default: @@ -4449,6 +4459,11 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case VNF_ADD_UN_OVF: case VNF_SUB_UN_OVF: case VNF_MUL_UN_OVF: + if (IsVNHandle(arg0VN) || IsVNHandle(arg1VN)) + { + return false; + } + break; case VNF_Cast: case VNF_CastOvf: From 3ced9a9b067b88c8b2e2bddf04c5eb02bc38f676 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 16 Feb 2024 16:00:05 +0100 Subject: [PATCH 2/5] Fix the conditions for null nodes --- src/coreclr/jit/valuenum.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index c2f8c1a3a97c18..ed530b1861e489 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4426,7 +4426,8 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case GT_GE: case GT_LT: case GT_LE: - if (IsVNHandle(arg0VN) || IsVNHandle(arg1VN)) + if ((IsVNHandle(arg0VN) && arg1VN != VNForNull()) || + (IsVNHandle(arg1VN) && arg0VN != VNForNull())) { return false; } @@ -4434,7 +4435,8 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case GT_EQ: case GT_NE: - if (IsVNHandle(arg0VN) != IsVNHandle(arg1VN)) + if ((IsVNHandle(arg0VN) || arg0VN == VNForNull()) != + (IsVNHandle(arg1VN) || arg1VN == VNForNull())) { return false; } @@ -4452,6 +4454,12 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case VNF_GE_UN: case VNF_LT_UN: case VNF_LE_UN: + if ((IsVNHandle(arg0VN) && arg1VN != VNForNull()) || + (IsVNHandle(arg1VN) && arg0VN != VNForNull())) + { + return false; + } + break; case VNF_ADD_OVF: case VNF_SUB_OVF: From 3d5131675d569946aebae06b61a19e0cf270b5f4 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 16 Feb 2024 16:02:30 +0100 Subject: [PATCH 3/5] Fix the last commit for non-GT/GE/LT/LE --- src/coreclr/jit/valuenum.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index ed530b1861e489..31b6e03d783574 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4421,6 +4421,11 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case GT_RSZ: case GT_ROL: case GT_ROR: + if (IsVNHandle(arg0VN) || IsVNHandle(arg1VN)) + { + return false; + } + break; case GT_GT: case GT_GE: From aec020f15571f9ee93ca5d077c61aeb01c1c5bb6 Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Fri, 16 Feb 2024 17:16:09 +0100 Subject: [PATCH 4/5] Make JIT format happy --- src/coreclr/jit/valuenum.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 31b6e03d783574..291f6b0d001e80 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4421,7 +4421,7 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case GT_RSZ: case GT_ROL: case GT_ROR: - if (IsVNHandle(arg0VN) || IsVNHandle(arg1VN)) + if (IsVNHandle(arg0VN) || IsVNHandle(arg1VN)) { return false; } @@ -4431,8 +4431,7 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case GT_GE: case GT_LT: case GT_LE: - if ((IsVNHandle(arg0VN) && arg1VN != VNForNull()) || - (IsVNHandle(arg1VN) && arg0VN != VNForNull())) + if ((IsVNHandle(arg0VN) && arg1VN != VNForNull()) || (IsVNHandle(arg1VN) && arg0VN != VNForNull())) { return false; } @@ -4440,8 +4439,7 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case GT_EQ: case GT_NE: - if ((IsVNHandle(arg0VN) || arg0VN == VNForNull()) != - (IsVNHandle(arg1VN) || arg1VN == VNForNull())) + if ((IsVNHandle(arg0VN) || arg0VN == VNForNull()) != (IsVNHandle(arg1VN) || arg1VN == VNForNull())) { return false; } @@ -4459,8 +4457,7 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case VNF_GE_UN: case VNF_LT_UN: case VNF_LE_UN: - if ((IsVNHandle(arg0VN) && arg1VN != VNForNull()) || - (IsVNHandle(arg1VN) && arg0VN != VNForNull())) + if ((IsVNHandle(arg0VN) && arg1VN != VNForNull()) || (IsVNHandle(arg1VN) && arg0VN != VNForNull())) { return false; } From 4d7744fb3f96f1b7dec71d9ca1cb17ea285c42ba Mon Sep 17 00:00:00 2001 From: Filip Navara Date: Sat, 17 Feb 2024 08:35:26 +0100 Subject: [PATCH 5/5] More conservative approach. Limit only arithmetic operations involving handles in a relocatable code. --- src/coreclr/jit/valuenum.cpp | 23 ++++------------------- 1 file changed, 4 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 291f6b0d001e80..5b2af35004cb6e 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4421,28 +4421,18 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case GT_RSZ: case GT_ROL: case GT_ROR: - if (IsVNHandle(arg0VN) || IsVNHandle(arg1VN)) + if (m_pComp->opts.compReloc && (IsVNHandle(arg0VN) || IsVNHandle(arg1VN))) { return false; } break; + case GT_EQ: + case GT_NE: case GT_GT: case GT_GE: case GT_LT: case GT_LE: - if ((IsVNHandle(arg0VN) && arg1VN != VNForNull()) || (IsVNHandle(arg1VN) && arg0VN != VNForNull())) - { - return false; - } - break; - - case GT_EQ: - case GT_NE: - if ((IsVNHandle(arg0VN) || arg0VN == VNForNull()) != (IsVNHandle(arg1VN) || arg1VN == VNForNull())) - { - return false; - } break; default: @@ -4457,11 +4447,6 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case VNF_GE_UN: case VNF_LT_UN: case VNF_LE_UN: - if ((IsVNHandle(arg0VN) && arg1VN != VNForNull()) || (IsVNHandle(arg1VN) && arg0VN != VNForNull())) - { - return false; - } - break; case VNF_ADD_OVF: case VNF_SUB_OVF: @@ -4469,7 +4454,7 @@ bool ValueNumStore::VNEvalCanFoldBinaryFunc(var_types type, VNFunc func, ValueNu case VNF_ADD_UN_OVF: case VNF_SUB_UN_OVF: case VNF_MUL_UN_OVF: - if (IsVNHandle(arg0VN) || IsVNHandle(arg1VN)) + if (m_pComp->opts.compReloc && (IsVNHandle(arg0VN) || IsVNHandle(arg1VN))) { return false; }