From 4f9d795f02606ddbb2f2d061c7e6585735f8aa4d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 17 Nov 2020 17:04:41 -0800 Subject: [PATCH 1/3] Import string.Empty as CNS_STR --- src/coreclr/jit/gentree.h | 5 +++++ src/coreclr/jit/importer.cpp | 17 ++++++++++++----- src/coreclr/jit/morph.cpp | 11 ++++++++++- 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 334a62f5374490..97c17f1b38b0e0 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -3146,6 +3146,11 @@ struct GenTreeStrCon : public GenTree unsigned gtSconCPX; CORINFO_MODULE_HANDLE gtScpHnd; + bool IsStringEmptyField() + { + return gtSconCPX == 0 && gtScpHnd == nullptr; + } + // Because this node can come from an inlined method we need to // have the scope handle, since it will become a helper call. GenTreeStrCon(unsigned sconCPX, CORINFO_MODULE_HANDLE mod DEBUGARG(bool largeNode = false)) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 606ca5360b8979..954be1933151c1 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3883,8 +3883,17 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, // Optimize `ldstr + String::get_Length()` to CNS_INT // e.g. "Hello".Length => 5 int length = -1; - LPCWSTR str = info.compCompHnd->getStringLiteral(op1->AsStrCon()->gtScpHnd, - op1->AsStrCon()->gtSconCPX, &length); + LPCWSTR str; + if (op1->AsStrCon()->IsStringEmptyField()) + { + str = L""; + length = 0; + } + else + { + str = info.compCompHnd->getStringLiteral(op1->AsStrCon()->gtScpHnd, op1->AsStrCon()->gtSconCPX, + &length); + } if (length >= 0) { retNode = gtNewIconNode(length); @@ -14851,9 +14860,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) { assert(aflags & CORINFO_ACCESS_GET); - LPVOID pValue; - InfoAccessType iat = info.compCompHnd->emptyStringLiteral(&pValue); - op1 = gtNewStringLiteralNode(iat, pValue); + op1 = gtNewSconNode(0, nullptr); goto FIELD_DONE; } break; diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index bc69b8cdeced68..189676f7621cbb 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -5453,7 +5453,8 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) if (opts.OptimizationEnabled() && asIndex->Arr()->OperIs(GT_CNS_STR) && asIndex->Index()->IsIntCnsFitsInI32()) { const int cnsIndex = static_cast(asIndex->Index()->AsIntConCommon()->IconValue()); - if (cnsIndex >= 0) + // NOTE: don't fold it for string.Empty[cns_index] or negative indices - it's going to crash anyway + if ((cnsIndex >= 0) && !asIndex->Arr()->AsStrCon()->IsStringEmptyField()) { int length; LPCWSTR str = info.compCompHnd->getStringLiteral(asIndex->Arr()->AsStrCon()->gtScpHnd, @@ -9276,12 +9277,20 @@ GenTree* Compiler::fgMorphConst(GenTree* tree) return tree; } + if (tree->AsStrCon()->IsStringEmptyField()) + { + LPVOID pValue; + InfoAccessType iat = info.compCompHnd->emptyStringLiteral(&pValue); + return fgMorphTree(gtNewStringLiteralNode(iat, pValue)); + } + // TODO-CQ: Do this for compCurBB->isRunRarely(). Doing that currently will // guarantee slow performance for that block. Instead cache the return value // of CORINFO_HELP_STRCNS and go to cache first giving reasonable perf. if (compCurBB->bbJumpKind == BBJ_THROW) { + assert(!tree->AsStrCon()->IsStringEmptyField()); CorInfoHelpFunc helper = info.compCompHnd->getLazyStringLiteralHelper(tree->AsStrCon()->gtScpHnd); if (helper != CORINFO_HELP_UNDEF) { From 32c449a0436362605e91fcb718d94382f4ae4583 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 18 Nov 2020 02:01:34 -0800 Subject: [PATCH 2/3] Change L"" to _T("") --- src/coreclr/jit/importer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 954be1933151c1..15ab85e6732e23 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3886,7 +3886,7 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, LPCWSTR str; if (op1->AsStrCon()->IsStringEmptyField()) { - str = L""; + str = _T(""); length = 0; } else From 4da40f2efb94269932b8fb6c1ebe75a49b8e74cb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 22 Dec 2020 00:23:28 +0300 Subject: [PATCH 3/3] Address feedback --- src/coreclr/jit/compiler.cpp | 2 ++ src/coreclr/jit/compiler.h | 2 ++ src/coreclr/jit/gentree.h | 5 ----- src/coreclr/jit/importer.cpp | 24 +++++++++++------------ src/coreclr/jit/morph.cpp | 37 ++++++++++++++++++++---------------- 5 files changed, 37 insertions(+), 33 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index b25afff5a6fdaa..97ae2304b965c4 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -1393,6 +1393,8 @@ bool Compiler::s_dspMemStats = false; const bool Compiler::Options::compNoPInvokeInlineCB = false; #endif +unsigned Compiler::s_emptyStringSconCPX = 0; + /***************************************************************************** * * One time initialization code diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 6539b755a45c09..6b94178449b4cc 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -9491,6 +9491,8 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX //-------------------------- Global Compiler Data ------------------------------------ + static unsigned s_emptyStringSconCPX; + #ifdef DEBUG private: static LONG s_compMethodsCount; // to produce unique label names diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 68767ac943067f..165c98ce2471a7 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -3148,11 +3148,6 @@ struct GenTreeStrCon : public GenTree unsigned gtSconCPX; CORINFO_MODULE_HANDLE gtScpHnd; - bool IsStringEmptyField() - { - return gtSconCPX == 0 && gtScpHnd == nullptr; - } - // Because this node can come from an inlined method we need to // have the scope handle, since it will become a helper call. GenTreeStrCon(unsigned sconCPX, CORINFO_MODULE_HANDLE mod DEBUGARG(bool largeNode = false)) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 2c52fb277a7694..7332ab7e4fcbf9 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3880,17 +3880,8 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, // Optimize `ldstr + String::get_Length()` to CNS_INT // e.g. "Hello".Length => 5 int length = -1; - LPCWSTR str; - if (op1->AsStrCon()->IsStringEmptyField()) - { - str = _T(""); - length = 0; - } - else - { - str = info.compCompHnd->getStringLiteral(op1->AsStrCon()->gtScpHnd, op1->AsStrCon()->gtSconCPX, - &length); - } + LPCWSTR str = info.compCompHnd->getStringLiteral(op1->AsStrCon()->gtScpHnd, + op1->AsStrCon()->gtSconCPX, &length); if (length >= 0) { retNode = gtNewIconNode(length); @@ -14851,7 +14842,16 @@ void Compiler::impImportBlockCode(BasicBlock* block) { assert(aflags & CORINFO_ACCESS_GET); - op1 = gtNewSconNode(0, nullptr); + if (s_emptyStringSconCPX != 0) + { + op1 = gtNewSconNode(s_emptyStringSconCPX, info.compCompHnd->getClassModule(impGetObjectClass())); + } + else + { + LPVOID pValue; + InfoAccessType iat = info.compCompHnd->emptyStringLiteral(&pValue); + op1 = gtNewStringLiteralNode(iat, pValue); + } goto FIELD_DONE; } break; diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 612328cbd5ff65..8f013f82e88665 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -5448,8 +5448,7 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) if (opts.OptimizationEnabled() && asIndex->Arr()->OperIs(GT_CNS_STR) && asIndex->Index()->IsIntCnsFitsInI32()) { const int cnsIndex = static_cast(asIndex->Index()->AsIntConCommon()->IconValue()); - // NOTE: don't fold it for string.Empty[cns_index] or negative indices - it's going to crash anyway - if ((cnsIndex >= 0) && !asIndex->Arr()->AsStrCon()->IsStringEmptyField()) + if (cnsIndex >= 0) { int length; LPCWSTR str = info.compCompHnd->getStringLiteral(asIndex->Arr()->AsStrCon()->gtScpHnd, @@ -9271,12 +9270,8 @@ GenTree* Compiler::fgMorphConst(GenTree* tree) return tree; } - if (tree->AsStrCon()->IsStringEmptyField()) - { - LPVOID pValue; - InfoAccessType iat = info.compCompHnd->emptyStringLiteral(&pValue); - return fgMorphTree(gtNewStringLiteralNode(iat, pValue)); - } + CORINFO_MODULE_HANDLE scpHnd = tree->AsStrCon()->gtScpHnd; + unsigned sconCPX = tree->AsStrCon()->gtSconCPX; // TODO-CQ: Do this for compCurBB->isRunRarely(). Doing that currently will // guarantee slow performance for that block. Instead cache the return value @@ -9284,8 +9279,7 @@ GenTree* Compiler::fgMorphConst(GenTree* tree) if (compCurBB->bbJumpKind == BBJ_THROW) { - assert(!tree->AsStrCon()->IsStringEmptyField()); - CorInfoHelpFunc helper = info.compCompHnd->getLazyStringLiteralHelper(tree->AsStrCon()->gtScpHnd); + CorInfoHelpFunc helper = info.compCompHnd->getLazyStringLiteralHelper(scpHnd); if (helper != CORINFO_HELP_UNDEF) { // For un-important blocks, we want to construct the string lazily @@ -9293,12 +9287,12 @@ GenTree* Compiler::fgMorphConst(GenTree* tree) GenTreeCall::Use* args; if (helper == CORINFO_HELP_STRCNS_CURRENT_MODULE) { - args = gtNewCallArgs(gtNewIconNode(RidFromToken(tree->AsStrCon()->gtSconCPX), TYP_INT)); + args = gtNewCallArgs(gtNewIconNode(RidFromToken(sconCPX), TYP_INT)); } else { - args = gtNewCallArgs(gtNewIconNode(RidFromToken(tree->AsStrCon()->gtSconCPX), TYP_INT), - gtNewIconEmbScpHndNode(tree->AsStrCon()->gtScpHnd)); + args = gtNewCallArgs(gtNewIconNode(RidFromToken(sconCPX), TYP_INT), + gtNewIconEmbScpHndNode(scpHnd)); } tree = gtNewHelperCallNode(helper, TYP_REF, args); @@ -9306,11 +9300,22 @@ GenTree* Compiler::fgMorphConst(GenTree* tree) } } - assert(tree->AsStrCon()->gtScpHnd == info.compScopeHnd || !IsUninitialized(tree->AsStrCon()->gtScpHnd)); + assert(scpHnd == info.compScopeHnd || !IsUninitialized(scpHnd)); + + // Save SconCPX to s_emptyStringSconCPX if it's "" (empty string). + if ((s_emptyStringSconCPX == 0) && + (scpHnd == info.compCompHnd->getClassModule(impGetObjectClass()))) + { + int length = -1; + info.compCompHnd->getStringLiteral(scpHnd, sconCPX, &length); + if (length == 0) + { + s_emptyStringSconCPX = sconCPX; + } + } LPVOID pValue; - InfoAccessType iat = - info.compCompHnd->constructStringLiteral(tree->AsStrCon()->gtScpHnd, tree->AsStrCon()->gtSconCPX, &pValue); + InfoAccessType iat = info.compCompHnd->constructStringLiteral(scpHnd, sconCPX, &pValue); tree = gtNewStringLiteralNode(iat, pValue);