From f990b9d4b4881966995b7a421a914f231891d992 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Wed, 27 Apr 2022 01:00:39 -0700 Subject: [PATCH 1/4] CSE nullcheck involving 'this' --- src/coreclr/jit/morph.cpp | 6 +++++- src/coreclr/jit/optcse.cpp | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 0272b91d73877c..e9145f8b8c2da8 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -5624,7 +5624,11 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) GenTree* lclVar = gtNewLclvNode(lclNum, objRefType); nullchk = gtNewNullCheck(lclVar, compCurBB); - nullchk->gtFlags |= GTF_DONT_CSE; // Don't try to create a CSE for these TYP_BYTE indirections + if (lclNum != info.compThisArg) + { + // Don't try to create a CSE for these TYP_BYTE indirections unless it was a "this" pointer. + nullchk->gtFlags |= GTF_DONT_CSE; + } if (asg) { diff --git a/src/coreclr/jit/optcse.cpp b/src/coreclr/jit/optcse.cpp index e3d478de0ccb0c..9e4e0f808694ef 100644 --- a/src/coreclr/jit/optcse.cpp +++ b/src/coreclr/jit/optcse.cpp @@ -3684,6 +3684,14 @@ bool Compiler::optIsCSEcandidate(GenTree* tree) case GT_RETURN: return false; // Currently the only special nodes that we hit // that we know that we don't want to CSE + case GT_NULLCHECK: + GenTreeLclVar* lclVar; + if (tree->AsUnOp()->gtGetOp1()->OperIs(GT_LCL_VAR)) + { + lclVar = tree->AsUnOp()->gtGetOp1()->AsLclVar(); + return lclVar->GetLclNum() == info.compThisArg; + } + return false; default: break; // Any new nodes that we might add later... From 68b4b2f67914be79b86e5a2d6ccacd1234fd85e1 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Wed, 27 Apr 2022 12:10:50 -0700 Subject: [PATCH 2/4] Move the nullcheck to hoistable only --- src/coreclr/jit/optcse.cpp | 8 -------- src/coreclr/jit/optimizer.cpp | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/optcse.cpp b/src/coreclr/jit/optcse.cpp index 9e4e0f808694ef..e3d478de0ccb0c 100644 --- a/src/coreclr/jit/optcse.cpp +++ b/src/coreclr/jit/optcse.cpp @@ -3684,14 +3684,6 @@ bool Compiler::optIsCSEcandidate(GenTree* tree) case GT_RETURN: return false; // Currently the only special nodes that we hit // that we know that we don't want to CSE - case GT_NULLCHECK: - GenTreeLclVar* lclVar; - if (tree->AsUnOp()->gtGetOp1()->OperIs(GT_LCL_VAR)) - { - lclVar = tree->AsUnOp()->gtGetOp1()->AsLclVar(); - return lclVar->GetLclNum() == info.compThisArg; - } - return false; default: break; // Any new nodes that we might add later... diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 84c75a2cbb63c1..7094fe4a07d290 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -6521,6 +6521,21 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* blo { return false; } + else if (node->OperIs(GT_NULLCHECK)) + { + GenTreeLclVar* lclVar; + if (node->AsUnOp()->gtGetOp1()->OperIs(GT_LCL_VAR)) + { + // If a null-check is for `this` object, it is safe to + // hoist it out of the loop. + // + // TODO-CQ: Identify more scenarios where we can hoist + // the null-checks + lclVar = node->AsUnOp()->gtGetOp1()->AsLclVar(); + return lclVar->GetLclNum() == m_compiler->info.compThisArg; + } + return false; + } // Tree must be a suitable CSE candidate for us to be able to hoist it. return m_compiler->optIsCSEcandidate(node); From fd9832209c89a54353d97effa463f23a06d32511 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Thu, 28 Apr 2022 16:15:54 -0700 Subject: [PATCH 3/4] Minor fix to display IG01 weight correctly --- src/coreclr/jit/codegencommon.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 8572b608e491dd..079734d5b4a5a1 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1739,6 +1739,7 @@ void CodeGen::genGenerateMachineCode() { compiler->opts.disAsm = true; } + compiler->compCurBB = compiler->fgFirstBB; if (compiler->opts.disAsm) { From d02af75217cd62467bca1bfb0b6e9ab3d413945d Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Thu, 28 Apr 2022 17:07:32 -0700 Subject: [PATCH 4/4] Hoist nullcheck for other objects as well --- src/coreclr/jit/morph.cpp | 6 ------ src/coreclr/jit/optimizer.cpp | 18 ++++++------------ 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index e9145f8b8c2da8..565d885bca9dd7 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -5624,12 +5624,6 @@ GenTree* Compiler::fgMorphField(GenTree* tree, MorphAddrContext* mac) GenTree* lclVar = gtNewLclvNode(lclNum, objRefType); nullchk = gtNewNullCheck(lclVar, compCurBB); - if (lclNum != info.compThisArg) - { - // Don't try to create a CSE for these TYP_BYTE indirections unless it was a "this" pointer. - nullchk->gtFlags |= GTF_DONT_CSE; - } - if (asg) { // Create the "comma" node. diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 7094fe4a07d290..3b18e0d57a95b3 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -6523,18 +6523,12 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* blo } else if (node->OperIs(GT_NULLCHECK)) { - GenTreeLclVar* lclVar; - if (node->AsUnOp()->gtGetOp1()->OperIs(GT_LCL_VAR)) - { - // If a null-check is for `this` object, it is safe to - // hoist it out of the loop. - // - // TODO-CQ: Identify more scenarios where we can hoist - // the null-checks - lclVar = node->AsUnOp()->gtGetOp1()->AsLclVar(); - return lclVar->GetLclNum() == m_compiler->info.compThisArg; - } - return false; + // If a null-check is for `this` object, it is safe to + // hoist it out of the loop. Assrtionprop will get rid + // of left over nullchecks present inside the loop. Also, + // since NULLCHECK has no value, it will never be CSE, + // hence this check is not present in optIsCSEcandidate(). + return true; } // Tree must be a suitable CSE candidate for us to be able to hoist it.