From b98b836474880a26571c723ec6a63dfe8dbc3a36 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Tue, 2 Mar 2021 13:08:04 -0800 Subject: [PATCH 1/2] Don't use the CoercedConstantValue value as the key, when we have a reloc --- src/coreclr/jit/optcse.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/coreclr/jit/optcse.cpp b/src/coreclr/jit/optcse.cpp index 031a01c5d95936..5a8aa6334f1880 100644 --- a/src/coreclr/jit/optcse.cpp +++ b/src/coreclr/jit/optcse.cpp @@ -500,6 +500,9 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) // we don't allow/expect it to be set when we need a reloc // assert((key & TARGET_SIGN_BIT) == 0); + + // Don't use the CoercedConstantValue value as the key + key = vnLibNorm; } } else // Not a GT_COMMA or a GT_CNS_INT From 84842e73617b5e4f5349193b1355c0109286b1e1 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 3 Mar 2021 19:08:17 -0800 Subject: [PATCH 2/2] Code review feedback and added comments clarifying the behavior when isSharedConst is true. --- src/coreclr/jit/optcse.cpp | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/optcse.cpp b/src/coreclr/jit/optcse.cpp index 5a8aa6334f1880..9697185569ac22 100644 --- a/src/coreclr/jit/optcse.cpp +++ b/src/coreclr/jit/optcse.cpp @@ -484,24 +484,25 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) else if (enableSharedConstCSE && tree->IsIntegralConst()) { assert(vnStore->IsVNConstant(vnLibNorm)); - key = vnStore->CoercedConstantValue(vnLibNorm); - // We don't share small offset constants when we require a reloc + // We don't share small offset constants when they require a reloc // if (!tree->AsIntConCommon()->ImmedValNeedsReloc(this)) { - // Make constants that have the same upper bits use the same key - key = Encode_Shared_Const_CSE_Value(key); - isSharedConst = true; + // Here we make constants that have the same upper bits use the same key + // + // We create a key that encodes just the upper bits of the constant by + // shifting out some of the low bits, (12 or 16 bits) + // + // This is the only case where the hash key is not a ValueNumber + // + size_t constVal = vnStore->CoercedConstantValue(vnLibNorm); + key = Encode_Shared_Const_CSE_Value(constVal); + isSharedConst = true; } else { - // Since we are using the sign bit as a discriminator - // we don't allow/expect it to be set when we need a reloc - // - assert((key & TARGET_SIGN_BIT) == 0); - - // Don't use the CoercedConstantValue value as the key + // Use the vnLibNorm value as the key key = vnLibNorm; } } @@ -510,6 +511,12 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) key = vnLibNorm; } + // Make sure that the result of Is_Shared_Const_CSE(key) matches isSharedConst + // Note that when isSharedConst is true then we require that the TARGET_SIGN_BIT is set in the key + // and otherwise we require that we never create a ValueNumber with the TARGET_SIGN_BIT set. + // + assert(isSharedConst == Is_Shared_Const_CSE(key)); + // Compute the hash value for the expression hval = optCSEKeyToHashIndex(key, optCSEhashSize);