From 6ce0a3b22b1917047eb018cb41cad0f9bce011b2 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 4 Dec 2025 00:51:28 +0100 Subject: [PATCH 01/17] Inline nullable allocators --- src/coreclr/jit/compiler.cpp | 2 ++ src/coreclr/jit/compiler.h | 4 +++- src/coreclr/jit/importer.cpp | 39 ++++++++++++++++++++++++++++++++++++ src/coreclr/jit/morph.cpp | 34 +++++++++++++++++++++++++++++-- 4 files changed, 76 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 7482fc2fe1ced8..adb74c90a5858d 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4349,6 +4349,8 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl return; } + DoPhase(this, PHASE_PATCHPOINTS, &Compiler::fgEarlyExpandQmarkNodes); + // If instrumenting, add block and class probes. // if (compileFlags->IsSet(JitFlags::JIT_FLAG_BBINSTR)) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 64d2c358ce62fb..bc3306c2f2e7e1 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5565,7 +5565,8 @@ class Compiler GenTreeQmark* fgGetTopLevelQmark(GenTree* expr, GenTree** ppDst = nullptr); bool fgExpandQmarkStmt(BasicBlock* block, Statement* stmt); - void fgExpandQmarkNodes(); + PhaseStatus fgEarlyExpandQmarkNodes(); + void fgExpandQmarkNodes(bool early = false); bool fgSimpleLowerCastOfSmpOp(LIR::Range& range, GenTreeCast* cast); bool fgSimpleLowerBswap16(LIR::Range& range, GenTree* op); @@ -7426,6 +7427,7 @@ class Compiler #define OMF_HAS_EXPANDABLE_CAST 0x00080000 // Method contains casts eligible for late expansion #define OMF_HAS_STACK_ARRAY 0x00100000 // Method contains stack allocated arrays #define OMF_HAS_BOUNDS_CHECKS 0x00200000 // Method contains bounds checks +#define OMF_HAS_EARLY_EXPAND_QMARKS 0x00400000 // Method contains bounds checks // clang-format on diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index e9e4bc20ee5931..bd3bcf4d4df3e2 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3414,6 +3414,45 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) // Look at what helper we should use. CorInfoHelpFunc boxHelper = info.compCompHnd->getBoxHelper(pResolvedToken->hClass); + if ((boxHelper == CORINFO_HELP_BOX_NULLABLE) && opts.OptimizationEnabled()) + { + CORINFO_CLASS_HANDLE typeToBox = info.compCompHnd->getTypeForBox(pResolvedToken->hClass); + CorInfoType valueType = info.compCompHnd->asCorInfoType(typeToBox); + if (impIsPrimitive(valueType)) + { + // TODO-CQ: consider expanding this for all types including structs. + GenTree* hasValue; + GenTree* value; + impLoadNullableFields(exprToBox, pResolvedToken->hClass, &hasValue, &value); + + CORINFO_RESOLVED_TOKEN tokenCopy = *pResolvedToken; + tokenCopy.hClass = typeToBox; + GenTree* obj = gtNewAllocObjNode(&tokenCopy, info.compMethodHnd, false); + + unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); + GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); + GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); + GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); + GenTree* storeData = gtNewStoreIndNode(JITtype2varType(valueType), dataPtr, value, GTF_IND_NONFAULTING); + GenTreeOp* copyData = gtNewOperNode(GT_COMMA, TYP_REF, storeData, gtCloneExpr(objLcl)); + GenTreeOp* allocRoot = gtNewOperNode(GT_COMMA, TYP_REF, storeAlloc, copyData); + + compCurBB->SetFlags(BBF_HAS_NEWOBJ); + optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_EXPAND_QMARKS; + + GenTree* cond = gtNewOperNode(GT_NE, TYP_INT, hasValue, gtNewIconNode(0)); + GenTreeColon* colon = gtNewColonNode(TYP_REF, allocRoot, gtNewNull()); + GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); + + const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); + impStoreToTemp(result, qmark, CHECK_SPILL_ALL); + lvaSetClass(result, typeToBox); + impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); + return; + } + } + // Determine what expansion to prefer. // // In size/time/debuggable constrained modes, the helper call diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index e2293dacb4d939..7baf14bc66b5de 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14642,6 +14642,13 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) } else { + while (trueExpr->OperIs(GT_COMMA)) + { + Statement* trueStmt = fgNewStmtFromTree(trueExpr->gtGetOp1(), stmt->GetDebugInfo()); + trueExpr = trueExpr->gtGetOp2(); + fgInsertStmtAtEnd(thenBlock, trueStmt); + } + if (dst != nullptr) { trueExpr = dst->OperIs(GT_STORE_LCL_FLD) ? gtNewStoreLclFldNode(dstLclNum, dst->TypeGet(), @@ -14665,6 +14672,13 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) } else { + while (falseExpr->OperIs(GT_COMMA)) + { + Statement* falseStmt = fgNewStmtFromTree(falseExpr->gtGetOp1(), stmt->GetDebugInfo()); + falseExpr = falseExpr->gtGetOp2(); + fgInsertStmtAtEnd(elseBlock, falseStmt); + } + if (dst != nullptr) { falseExpr = @@ -14688,13 +14702,25 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) return introducedThrow; } +PhaseStatus Compiler::fgEarlyExpandQmarkNodes() +{ + if ((optMethodFlags & OMF_HAS_EARLY_EXPAND_QMARKS) == 0) + { + return PhaseStatus::MODIFIED_NOTHING; + } + + fgExpandQmarkNodes(true); + + return PhaseStatus::MODIFIED_EVERYTHING; +} + /***************************************************************************** * * Expand GT_QMARK nodes from the flow graph into basic blocks. * */ -void Compiler::fgExpandQmarkNodes() +void Compiler::fgExpandQmarkNodes(bool early) { bool introducedThrows = false; @@ -14715,7 +14741,11 @@ void Compiler::fgExpandQmarkNodes() fgPostExpandQmarkChecks(); #endif } - compQmarkRationalized = true; + + if (!early) + { + compQmarkRationalized = true; + } // TODO: if qmark expansion created throw blocks, try and merge them // From 8802daaa1b9c1d2c4b897de69ba668d064842334 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 4 Dec 2025 02:59:09 +0100 Subject: [PATCH 02/17] Update importer.cpp --- src/coreclr/jit/importer.cpp | 56 +++++++++++++++++++----------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index bd3bcf4d4df3e2..5296a6518baeaa 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3420,36 +3420,38 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) CorInfoType valueType = info.compCompHnd->asCorInfoType(typeToBox); if (impIsPrimitive(valueType)) { - // TODO-CQ: consider expanding this for all types including structs. - GenTree* hasValue; - GenTree* value; - impLoadNullableFields(exprToBox, pResolvedToken->hClass, &hasValue, &value); - CORINFO_RESOLVED_TOKEN tokenCopy = *pResolvedToken; tokenCopy.hClass = typeToBox; GenTree* obj = gtNewAllocObjNode(&tokenCopy, info.compMethodHnd, false); - - unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); - GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); - GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); - GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); - GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); - GenTree* storeData = gtNewStoreIndNode(JITtype2varType(valueType), dataPtr, value, GTF_IND_NONFAULTING); - GenTreeOp* copyData = gtNewOperNode(GT_COMMA, TYP_REF, storeData, gtCloneExpr(objLcl)); - GenTreeOp* allocRoot = gtNewOperNode(GT_COMMA, TYP_REF, storeAlloc, copyData); - - compCurBB->SetFlags(BBF_HAS_NEWOBJ); - optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_EXPAND_QMARKS; - - GenTree* cond = gtNewOperNode(GT_NE, TYP_INT, hasValue, gtNewIconNode(0)); - GenTreeColon* colon = gtNewColonNode(TYP_REF, allocRoot, gtNewNull()); - GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); - - const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); - impStoreToTemp(result, qmark, CHECK_SPILL_ALL); - lvaSetClass(result, typeToBox); - impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); - return; + if (obj != nullptr) + { + // TODO-CQ: consider expanding this for all types including structs. + GenTree* hasValue; + GenTree* value; + impLoadNullableFields(exprToBox, pResolvedToken->hClass, &hasValue, &value); + + unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); + GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); + GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); + GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); + GenTree* storeData = gtNewStoreIndNode(JITtype2varType(valueType), dataPtr, value, GTF_IND_NONFAULTING); + GenTreeOp* copyData = gtNewOperNode(GT_COMMA, TYP_REF, storeData, gtCloneExpr(objLcl)); + GenTreeOp* allocRoot = gtNewOperNode(GT_COMMA, TYP_REF, storeAlloc, copyData); + + compCurBB->SetFlags(BBF_HAS_NEWOBJ); + optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_EXPAND_QMARKS; + + GenTree* cond = gtNewOperNode(GT_NE, TYP_INT, hasValue, gtNewIconNode(0)); + GenTreeColon* colon = gtNewColonNode(TYP_REF, allocRoot, gtNewNull()); + GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); + + const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); + impStoreToTemp(result, qmark, CHECK_SPILL_ALL); + lvaSetClass(result, typeToBox); + impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); + return; + } } } From 8292f7bd13ebbdaebd2b55978edccd9b15cb8af3 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 4 Dec 2025 10:07:34 +0100 Subject: [PATCH 03/17] test --- src/coreclr/jit/importer.cpp | 53 ++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 5296a6518baeaa..b96299566b7e78 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3425,32 +3425,33 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) GenTree* obj = gtNewAllocObjNode(&tokenCopy, info.compMethodHnd, false); if (obj != nullptr) { - // TODO-CQ: consider expanding this for all types including structs. - GenTree* hasValue; - GenTree* value; - impLoadNullableFields(exprToBox, pResolvedToken->hClass, &hasValue, &value); - - unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); - GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); - GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); - GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); - GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); - GenTree* storeData = gtNewStoreIndNode(JITtype2varType(valueType), dataPtr, value, GTF_IND_NONFAULTING); - GenTreeOp* copyData = gtNewOperNode(GT_COMMA, TYP_REF, storeData, gtCloneExpr(objLcl)); - GenTreeOp* allocRoot = gtNewOperNode(GT_COMMA, TYP_REF, storeAlloc, copyData); - - compCurBB->SetFlags(BBF_HAS_NEWOBJ); - optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_EXPAND_QMARKS; - - GenTree* cond = gtNewOperNode(GT_NE, TYP_INT, hasValue, gtNewIconNode(0)); - GenTreeColon* colon = gtNewColonNode(TYP_REF, allocRoot, gtNewNull()); - GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); - - const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); - impStoreToTemp(result, qmark, CHECK_SPILL_ALL); - lvaSetClass(result, typeToBox); - impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); - return; + //// TODO-CQ: consider expanding this for all types including structs. + // GenTree* hasValue; + // GenTree* value; + // impLoadNullableFields(exprToBox, pResolvedToken->hClass, &hasValue, &value); + + // unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); + // GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); + // GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); + // GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + // GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); + // GenTree* storeData = gtNewStoreIndNode(JITtype2varType(valueType), dataPtr, value, + // GTF_IND_NONFAULTING); GenTreeOp* copyData = gtNewOperNode(GT_COMMA, TYP_REF, storeData, + // gtCloneExpr(objLcl)); GenTreeOp* allocRoot = gtNewOperNode(GT_COMMA, TYP_REF, storeAlloc, copyData); + + // compCurBB->SetFlags(BBF_HAS_NEWOBJ); + // optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_EXPAND_QMARKS; + + // GenTree* cond = gtNewOperNode(GT_NE, TYP_INT, hasValue, gtNewIconNode(0)); + // GenTreeColon* colon = gtNewColonNode(TYP_REF, allocRoot, gtNewNull()); + // GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); + + // const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); + // impStoreToTemp(result, qmark, CHECK_SPILL_ALL); + // lvaSetClass(result, typeToBox); + // impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); + // return; + optMethodFlags |= OMF_HAS_EARLY_EXPAND_QMARKS; } } } From 6fb8a3e24d9b4804efcdeb7bf9abed0212c2e89a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 4 Dec 2025 12:34:45 +0100 Subject: [PATCH 04/17] test --- src/coreclr/jit/importer.cpp | 68 +++++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 28 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index b96299566b7e78..19d42a709f1583 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3414,7 +3414,7 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) // Look at what helper we should use. CorInfoHelpFunc boxHelper = info.compCompHnd->getBoxHelper(pResolvedToken->hClass); - if ((boxHelper == CORINFO_HELP_BOX_NULLABLE) && opts.OptimizationEnabled()) + if ((boxHelper == CORINFO_HELP_BOX_NULLABLE) && opts.OptimizationEnabled() && !compCurBB->isRunRarely()) { CORINFO_CLASS_HANDLE typeToBox = info.compCompHnd->getTypeForBox(pResolvedToken->hClass); CorInfoType valueType = info.compCompHnd->asCorInfoType(typeToBox); @@ -3425,33 +3425,45 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) GenTree* obj = gtNewAllocObjNode(&tokenCopy, info.compMethodHnd, false); if (obj != nullptr) { - //// TODO-CQ: consider expanding this for all types including structs. - // GenTree* hasValue; - // GenTree* value; - // impLoadNullableFields(exprToBox, pResolvedToken->hClass, &hasValue, &value); - - // unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); - // GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); - // GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); - // GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); - // GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); - // GenTree* storeData = gtNewStoreIndNode(JITtype2varType(valueType), dataPtr, value, - // GTF_IND_NONFAULTING); GenTreeOp* copyData = gtNewOperNode(GT_COMMA, TYP_REF, storeData, - // gtCloneExpr(objLcl)); GenTreeOp* allocRoot = gtNewOperNode(GT_COMMA, TYP_REF, storeAlloc, copyData); - - // compCurBB->SetFlags(BBF_HAS_NEWOBJ); - // optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_EXPAND_QMARKS; - - // GenTree* cond = gtNewOperNode(GT_NE, TYP_INT, hasValue, gtNewIconNode(0)); - // GenTreeColon* colon = gtNewColonNode(TYP_REF, allocRoot, gtNewNull()); - // GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); - - // const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); - // impStoreToTemp(result, qmark, CHECK_SPILL_ALL); - // lvaSetClass(result, typeToBox); - // impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); - // return; - optMethodFlags |= OMF_HAS_EARLY_EXPAND_QMARKS; + // TODO-CQ: consider expanding this for all types including structs. + GenTree* hasValue; + GenTree* value; + impLoadNullableFields(exprToBox, pResolvedToken->hClass, &hasValue, &value); + + var_types dstTyp = JITtype2varType(valueType); + var_types srcTyp = value->TypeGet(); + + // We allow float <-> double mismatches and implicit truncation for small types. + assert((genActualType(srcTyp) == genActualType(dstTyp)) || + (varTypeIsFloating(srcTyp) == varTypeIsFloating(dstTyp))); + + if (srcTyp != dstTyp) + { + value = gtNewCastNode(genActualType(dstTyp), value, false, dstTyp); + } + + unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); + GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); + GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); + GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); + GenTree* storeData = gtNewStoreIndNode(JITtype2varType(valueType), dataPtr, value, GTF_IND_NONFAULTING); + GenTreeOp* copyData = gtNewOperNode(GT_COMMA, TYP_REF, storeData, gtCloneExpr(objLcl)); + GenTreeOp* allocRoot = gtNewOperNode(GT_COMMA, TYP_REF, storeAlloc, copyData); + + compCurBB->SetFlags(BBF_HAS_NEWOBJ); + optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_EXPAND_QMARKS; + + GenTree* cond = gtNewOperNode(GT_NE, TYP_INT, hasValue, gtNewIconNode(0)); + GenTreeColon* colon = gtNewColonNode(TYP_REF, allocRoot, gtNewNull()); + GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); + + const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); + impStoreToTemp(result, qmark, CHECK_SPILL_ALL); + lvaSetClass(result, typeToBox, true); + lvaSetClass(objLclNum, typeToBox, true); + impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); + return; } } } From 147653ddebe0e31568a27faf96ddc5998b154cdb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 4 Dec 2025 21:23:10 +0100 Subject: [PATCH 05/17] fix naot --- src/coreclr/jit/importer.cpp | 83 +++++++++++++++++------------------- 1 file changed, 38 insertions(+), 45 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 19d42a709f1583..9f5433f765c7d7 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3420,51 +3420,44 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) CorInfoType valueType = info.compCompHnd->asCorInfoType(typeToBox); if (impIsPrimitive(valueType)) { - CORINFO_RESOLVED_TOKEN tokenCopy = *pResolvedToken; - tokenCopy.hClass = typeToBox; - GenTree* obj = gtNewAllocObjNode(&tokenCopy, info.compMethodHnd, false); - if (obj != nullptr) - { - // TODO-CQ: consider expanding this for all types including structs. - GenTree* hasValue; - GenTree* value; - impLoadNullableFields(exprToBox, pResolvedToken->hClass, &hasValue, &value); - - var_types dstTyp = JITtype2varType(valueType); - var_types srcTyp = value->TypeGet(); - - // We allow float <-> double mismatches and implicit truncation for small types. - assert((genActualType(srcTyp) == genActualType(dstTyp)) || - (varTypeIsFloating(srcTyp) == varTypeIsFloating(dstTyp))); - - if (srcTyp != dstTyp) - { - value = gtNewCastNode(genActualType(dstTyp), value, false, dstTyp); - } - - unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); - GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); - GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); - GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); - GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); - GenTree* storeData = gtNewStoreIndNode(JITtype2varType(valueType), dataPtr, value, GTF_IND_NONFAULTING); - GenTreeOp* copyData = gtNewOperNode(GT_COMMA, TYP_REF, storeData, gtCloneExpr(objLcl)); - GenTreeOp* allocRoot = gtNewOperNode(GT_COMMA, TYP_REF, storeAlloc, copyData); - - compCurBB->SetFlags(BBF_HAS_NEWOBJ); - optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_EXPAND_QMARKS; - - GenTree* cond = gtNewOperNode(GT_NE, TYP_INT, hasValue, gtNewIconNode(0)); - GenTreeColon* colon = gtNewColonNode(TYP_REF, allocRoot, gtNewNull()); - GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); - - const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); - impStoreToTemp(result, qmark, CHECK_SPILL_ALL); - lvaSetClass(result, typeToBox, true); - lvaSetClass(objLclNum, typeToBox, true); - impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); - return; - } + bool helperHasSideEffects; + CorInfoHelpFunc helper = info.compCompHnd->getNewHelper(typeToBox, &helperHasSideEffects); + GenTreeAllocObj* obj = + gtNewAllocObjNode(helper, true, typeToBox, TYP_REF, gtNewIconEmbClsHndNode(typeToBox)); + + GenTreeFlags indirFlags = GTF_EMPTY; + exprToBox = impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags); + CORINFO_FIELD_HANDLE valueFldHnd = info.compCompHnd->getFieldInClass(pResolvedToken->hClass, 1); + static_assert(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); + GenTree* hasValue = gtNewLoadValueNode(TYP_UBYTE, nullptr, gtCloneExpr(exprToBox)); + GenTree* valueOffset = gtNewIconNode(info.compCompHnd->getFieldOffset(valueFldHnd), TYP_I_IMPL); + GenTree* valueAddr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(exprToBox), valueOffset); + GenTree* value = gtNewLoadValueNode(JITtype2varType(valueType), nullptr, valueAddr); + + unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); + GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); + GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); + GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); + GenTree* storeData = + gtNewStoreIndNode(JITtype2varType(valueType), dataPtr, gtCloneExpr(value), GTF_IND_NONFAULTING); + GenTreeOp* copyData = gtNewOperNode(GT_COMMA, TYP_REF, storeData, gtCloneExpr(objLcl)); + GenTreeOp* allocRoot = gtNewOperNode(GT_COMMA, TYP_REF, storeAlloc, copyData); + + compCurBB->SetFlags(BBF_HAS_NEWOBJ); + optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_EXPAND_QMARKS; + + GenTree* cond = gtNewOperNode(GT_EQ, TYP_INT, hasValue, gtNewIconNode(0)); + GenTreeColon* colon = gtNewColonNode(TYP_REF, gtNewNull(), allocRoot); + GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); + + const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); + impStoreToTemp(result, qmark, CHECK_SPILL_ALL); + lvaSetClass(result, typeToBox, true); + + lvaSetClass(objLclNum, typeToBox, true); + impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); + return; } } From 17d3f608716963f51bb027270afa8105d5ce8631 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 4 Dec 2025 23:47:52 +0100 Subject: [PATCH 06/17] enable for all types --- src/coreclr/jit/compiler.cpp | 6 ++-- src/coreclr/jit/compiler.h | 5 ++-- src/coreclr/jit/compphases.h | 1 + src/coreclr/jit/importer.cpp | 58 +++++++++++++++++++++++++----------- src/coreclr/jit/morph.cpp | 40 ++++++++++++++----------- 5 files changed, 69 insertions(+), 41 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index adb74c90a5858d..6d7c51de57552b 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4349,7 +4349,9 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl return; } - DoPhase(this, PHASE_PATCHPOINTS, &Compiler::fgEarlyExpandQmarkNodes); + DoPhase(this, PHASE_EARLY_QMARK_EXPANSION, [this]() { + return fgExpandQmarkNodes(/*early*/ true); + }); // If instrumenting, add block and class probes. // @@ -4548,7 +4550,7 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl // Decide the kind of code we want to generate fgSetOptions(); - fgExpandQmarkNodes(); + fgExpandQmarkNodes(/*early*/ false); #ifdef DEBUG compCurBB = nullptr; diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index bc3306c2f2e7e1..7dc0c45392c03a 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5565,8 +5565,7 @@ class Compiler GenTreeQmark* fgGetTopLevelQmark(GenTree* expr, GenTree** ppDst = nullptr); bool fgExpandQmarkStmt(BasicBlock* block, Statement* stmt); - PhaseStatus fgEarlyExpandQmarkNodes(); - void fgExpandQmarkNodes(bool early = false); + PhaseStatus fgExpandQmarkNodes(bool early); bool fgSimpleLowerCastOfSmpOp(LIR::Range& range, GenTreeCast* cast); bool fgSimpleLowerBswap16(LIR::Range& range, GenTree* op); @@ -7427,7 +7426,7 @@ class Compiler #define OMF_HAS_EXPANDABLE_CAST 0x00080000 // Method contains casts eligible for late expansion #define OMF_HAS_STACK_ARRAY 0x00100000 // Method contains stack allocated arrays #define OMF_HAS_BOUNDS_CHECKS 0x00200000 // Method contains bounds checks -#define OMF_HAS_EARLY_EXPAND_QMARKS 0x00400000 // Method contains bounds checks +#define OMF_HAS_EARLY_QMARKS 0x00400000 // Method contains early expandable QMARKs // clang-format on diff --git a/src/coreclr/jit/compphases.h b/src/coreclr/jit/compphases.h index c284ba905f4542..191ed978601c69 100644 --- a/src/coreclr/jit/compphases.h +++ b/src/coreclr/jit/compphases.h @@ -63,6 +63,7 @@ CompPhaseNameMacro(PHASE_GS_COOKIE, "GS Cookie", CompPhaseNameMacro(PHASE_COMPUTE_BLOCK_WEIGHTS, "Compute block weights", false, -1, false) CompPhaseNameMacro(PHASE_CREATE_FUNCLETS, "Create EH funclets", false, -1, false) CompPhaseNameMacro(PHASE_HEAD_TAIL_MERGE, "Head and tail merge", false, -1, false) +CompPhaseNameMacro(PHASE_EARLY_QMARK_EXPANSION, "Early QMARK expansion", false, -1, false) CompPhaseNameMacro(PHASE_MERGE_THROWS, "Merge throw blocks", false, -1, false) CompPhaseNameMacro(PHASE_INVERT_LOOPS, "Invert loops", false, -1, false) CompPhaseNameMacro(PHASE_HEAD_TAIL_MERGE2, "Post-morph head and tail merge", false, -1, false) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 9f5433f765c7d7..7dcb436567d7ef 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3414,47 +3414,69 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) // Look at what helper we should use. CorInfoHelpFunc boxHelper = info.compCompHnd->getBoxHelper(pResolvedToken->hClass); + // Expand nullable boxes inline in optimized code in hot paths, it's slightly different from + // other expansions since we have to use QMARK to return null for nullables with no value. + // + // obj = nullable._hasValue == 0 ? null : (boxed underlying value) + // if ((boxHelper == CORINFO_HELP_BOX_NULLABLE) && opts.OptimizationEnabled() && !compCurBB->isRunRarely()) { CORINFO_CLASS_HANDLE typeToBox = info.compCompHnd->getTypeForBox(pResolvedToken->hClass); - CorInfoType valueType = info.compCompHnd->asCorInfoType(typeToBox); - if (impIsPrimitive(valueType)) - { - bool helperHasSideEffects; - CorInfoHelpFunc helper = info.compCompHnd->getNewHelper(typeToBox, &helperHasSideEffects); - GenTreeAllocObj* obj = - gtNewAllocObjNode(helper, true, typeToBox, TYP_REF, gtNewIconEmbClsHndNode(typeToBox)); - GenTreeFlags indirFlags = GTF_EMPTY; - exprToBox = impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags); - CORINFO_FIELD_HANDLE valueFldHnd = info.compCompHnd->getFieldInClass(pResolvedToken->hClass, 1); - static_assert(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); - GenTree* hasValue = gtNewLoadValueNode(TYP_UBYTE, nullptr, gtCloneExpr(exprToBox)); - GenTree* valueOffset = gtNewIconNode(info.compCompHnd->getFieldOffset(valueFldHnd), TYP_I_IMPL); - GenTree* valueAddr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(exprToBox), valueOffset); - GenTree* value = gtNewLoadValueNode(JITtype2varType(valueType), nullptr, valueAddr); + // We need to compose a new CORINFO_RESOLVED_TOKEN for the underlying (typeToBox) type + CORINFO_RESOLVED_TOKEN tk = *pResolvedToken; + tk.hClass = typeToBox; + tk.tokenType = CORINFO_TOKENKIND_Casting; + GenTree* obj = gtNewAllocObjNode(&tk, info.compMethodHnd, false); + if (obj != nullptr) + { + // First, decompose the Nullable<> into _hasValue and _value fields. + // + GenTreeFlags indirFlags = GTF_EMPTY; + exprToBox = impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags); + CORINFO_FIELD_HANDLE valueFldHnd = info.compCompHnd->getFieldInClass(pResolvedToken->hClass, 1); + CORINFO_CLASS_HANDLE valueStructCls = NO_CLASS_HANDLE; + static_assert(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); + unsigned cnsValueOffset = info.compCompHnd->getFieldOffset(valueFldHnd); + ClassLayout* layout = nullptr; + CorInfoType corFldType = info.compCompHnd->getFieldType(valueFldHnd, &valueStructCls); + var_types valueType = TypeHandleToVarType(corFldType, valueStructCls, &layout); + GenTree* valueOffset = gtNewIconNode(cnsValueOffset, TYP_I_IMPL); + GenTree* valueAddr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(exprToBox), valueOffset); + GenTree* value = gtNewLoadValueNode(valueType, layout, valueAddr); + GenTree* hasValue = gtNewLoadValueNode(TYP_UBYTE, nullptr, gtCloneExpr(exprToBox)); + + // Now we need to copy value into the allocated box + // unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); GenTree* storeData = - gtNewStoreIndNode(JITtype2varType(valueType), dataPtr, gtCloneExpr(value), GTF_IND_NONFAULTING); + gtNewStoreValueNode(valueType, layout, dataPtr, gtCloneExpr(value), GTF_IND_NONFAULTING); + + // Wrap it all in two commas, it will look like: + // lcl = allocobj + // *(lcl + sizeof(void*)) = value + // lcl + // GenTreeOp* copyData = gtNewOperNode(GT_COMMA, TYP_REF, storeData, gtCloneExpr(objLcl)); GenTreeOp* allocRoot = gtNewOperNode(GT_COMMA, TYP_REF, storeAlloc, copyData); + // QMARK expansion will propagate block flags properly. compCurBB->SetFlags(BBF_HAS_NEWOBJ); - optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_EXPAND_QMARKS; + optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_QMARKS; GenTree* cond = gtNewOperNode(GT_EQ, TYP_INT, hasValue, gtNewIconNode(0)); GenTreeColon* colon = gtNewColonNode(TYP_REF, gtNewNull(), allocRoot); GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); + // QMARK has to be a top-level statement const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); impStoreToTemp(result, qmark, CHECK_SPILL_ALL); lvaSetClass(result, typeToBox, true); - lvaSetClass(objLclNum, typeToBox, true); impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); return; diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 7baf14bc66b5de..e4b3546e9349f2 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14702,26 +14702,31 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) return introducedThrow; } -PhaseStatus Compiler::fgEarlyExpandQmarkNodes() +//------------------------------------------------------------------------ +// fgExpandQmarkNodes: expand all QMARK nodes into control flow. +// +// Arguments: +// early - whether this is the early expansion phase. +// +// Returns: +// Suitable phase status. +// +PhaseStatus Compiler::fgExpandQmarkNodes(bool early) { - if ((optMethodFlags & OMF_HAS_EARLY_EXPAND_QMARKS) == 0) + if (early && ((optMethodFlags & OMF_HAS_EARLY_QMARKS) == 0)) { return PhaseStatus::MODIFIED_NOTHING; } - fgExpandQmarkNodes(true); - - return PhaseStatus::MODIFIED_EVERYTHING; -} - -/***************************************************************************** - * - * Expand GT_QMARK nodes from the flow graph into basic blocks. - * - */ + // We don't spawn more QMARKs after the early expansion phase, + // Eventually we might want to get rid of the late expansion phase, but currently + // it produces too many regressions (mostly because of ForwardSub). + if (compQmarkRationalized) + { + assert(!early); + return PhaseStatus::MODIFIED_NOTHING; + } -void Compiler::fgExpandQmarkNodes(bool early) -{ bool introducedThrows = false; if (compQmarkUsed) @@ -14742,10 +14747,7 @@ void Compiler::fgExpandQmarkNodes(bool early) #endif } - if (!early) - { - compQmarkRationalized = true; - } + compQmarkRationalized = true; // TODO: if qmark expansion created throw blocks, try and merge them // @@ -14753,6 +14755,8 @@ void Compiler::fgExpandQmarkNodes(bool early) { JITDUMP("Qmark expansion created new throw blocks\n"); } + + return PhaseStatus::MODIFIED_EVERYTHING; } //------------------------------------------------------------------------ From c9bd019958880af423614ab2988d0dd5f05c8b5c Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 4 Dec 2025 23:52:23 +0100 Subject: [PATCH 07/17] add comments --- src/coreclr/jit/morph.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index e4b3546e9349f2..ca1f026fe5b272 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14642,6 +14642,7 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) } else { + // There is no good reason to keep root COMMAs when we can split them into separate statements. while (trueExpr->OperIs(GT_COMMA)) { Statement* trueStmt = fgNewStmtFromTree(trueExpr->gtGetOp1(), stmt->GetDebugInfo()); @@ -14672,6 +14673,7 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) } else { + // There is no good reason to keep root COMMAs when we can split them into separate statements. while (falseExpr->OperIs(GT_COMMA)) { Statement* falseStmt = fgNewStmtFromTree(falseExpr->gtGetOp1(), stmt->GetDebugInfo()); @@ -14706,7 +14708,8 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) // fgExpandQmarkNodes: expand all QMARK nodes into control flow. // // Arguments: -// early - whether this is the early expansion phase. +// early - whether this is the early expansion phase. Late expansion +// happens too late for some optimizations, e.g. object stack-alloc. // // Returns: // Suitable phase status. From 6384a5e404aacf0abae7d244d93c128bd53d114e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 5 Dec 2025 00:47:25 +0100 Subject: [PATCH 08/17] disable shared generics --- src/coreclr/jit/importer.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 7dcb436567d7ef..020eabf4d4d1f6 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3419,7 +3419,8 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) // // obj = nullable._hasValue == 0 ? null : (boxed underlying value) // - if ((boxHelper == CORINFO_HELP_BOX_NULLABLE) && opts.OptimizationEnabled() && !compCurBB->isRunRarely()) + if ((boxHelper == CORINFO_HELP_BOX_NULLABLE) && opts.OptimizationEnabled() && !compCurBB->isRunRarely() && + !eeIsSharedInst(pResolvedToken->hClass)) { CORINFO_CLASS_HANDLE typeToBox = info.compCompHnd->getTypeForBox(pResolvedToken->hClass); From 926ac20e27baaebc86bd303d1ed35569791edc5e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 5 Dec 2025 01:55:06 +0100 Subject: [PATCH 09/17] expand qmarks twice --- src/coreclr/jit/morph.cpp | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index ca1f026fe5b272..048070d11b22c7 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14721,15 +14721,6 @@ PhaseStatus Compiler::fgExpandQmarkNodes(bool early) return PhaseStatus::MODIFIED_NOTHING; } - // We don't spawn more QMARKs after the early expansion phase, - // Eventually we might want to get rid of the late expansion phase, but currently - // it produces too many regressions (mostly because of ForwardSub). - if (compQmarkRationalized) - { - assert(!early); - return PhaseStatus::MODIFIED_NOTHING; - } - bool introducedThrows = false; if (compQmarkUsed) From 44e2f7aca0ac334f00b7b513b0908d7ed4233656 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 5 Dec 2025 13:10:47 +0100 Subject: [PATCH 10/17] Clean up --- src/coreclr/jit/compiler.h | 1 + src/coreclr/jit/importer.cpp | 160 ++++++++++++++++++++--------------- src/coreclr/jit/morph.cpp | 2 +- 3 files changed, 94 insertions(+), 69 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 7dc0c45392c03a..dd0b8271a8731b 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4597,6 +4597,7 @@ class Compiler const BYTE* codeEndp, BoxPatterns opts); void impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken); + bool impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedToken); void impImportNewObjArray(CORINFO_RESOLVED_TOKEN* pResolvedToken, CORINFO_CALL_INFO* pCallInfo); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 020eabf4d4d1f6..86833b043c1c16 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3355,6 +3355,90 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, return -1; } +//------------------------------------------------------------------------ +// impImportAndPushBoxForNullable: import a "box Nullable<>" as an inlined sequence +// +// Arguments: +// pResolvedToken - resolved token from the box operation +// +// Return Value: +// true if inlined, false otherwise. +// +bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedToken) +{ + assert(info.compCompHnd->getBoxHelper(pResolvedToken->hClass) == CORINFO_HELP_BOX_NULLABLE); + + if (opts.OptimizationDisabled() || compCurBB->isRunRarely() || eeIsSharedInst(pResolvedToken->hClass)) + { + return false; + } + + CORINFO_CLASS_HANDLE typeToBox = info.compCompHnd->getTypeForBox(pResolvedToken->hClass); + + // We need to compose a new CORINFO_RESOLVED_TOKEN for the underlying (typeToBox) type + // E.g. if we are boxing Nullable, typeToBox is System.Int32. + CORINFO_RESOLVED_TOKEN tk = *pResolvedToken; + tk.hClass = typeToBox; + tk.tokenType = CORINFO_TOKENKIND_Casting; + GenTree* obj = gtNewAllocObjNode(&tk, info.compMethodHnd, false); + + if (obj == nullptr) + { + return false; + } + + GenTree* exprToBox = impPopStack().val; + + // First, decompose the Nullable<> into _hasValue and _value fields. + // + GenTreeFlags indirFlags = GTF_EMPTY; + exprToBox = impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags); + CORINFO_FIELD_HANDLE valueFldHnd = info.compCompHnd->getFieldInClass(pResolvedToken->hClass, 1); + CORINFO_CLASS_HANDLE valueStructCls = NO_CLASS_HANDLE; + static_assert(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); + unsigned cnsValueOffset = info.compCompHnd->getFieldOffset(valueFldHnd); + ClassLayout* layout = nullptr; + CorInfoType corFldType = info.compCompHnd->getFieldType(valueFldHnd, &valueStructCls); + var_types valueType = TypeHandleToVarType(corFldType, valueStructCls, &layout); + GenTree* valueOffset = gtNewIconNode(cnsValueOffset, TYP_I_IMPL); + GenTree* valueAddr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(exprToBox), valueOffset); + GenTree* value = gtNewLoadValueNode(valueType, layout, valueAddr); + GenTree* hasValue = gtNewLoadValueNode(TYP_UBYTE, nullptr, gtCloneExpr(exprToBox)); + + // Now we need to copy value into the allocated box + // + unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); + GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); + GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); + GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); + GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); + GenTree* storeData = gtNewStoreValueNode(valueType, layout, dataPtr, gtCloneExpr(value), GTF_IND_NONFAULTING); + + // Wrap it all in two commas, it will look like: + // lcl = allocobj + // *(lcl + sizeof(void*)) = value + // lcl + // + GenTreeOp* copyData = gtNewOperNode(GT_COMMA, TYP_REF, storeData, gtCloneExpr(objLcl)); + GenTreeOp* allocRoot = gtNewOperNode(GT_COMMA, TYP_REF, storeAlloc, copyData); + + // QMARK expansion will propagate block flags properly. + compCurBB->SetFlags(BBF_HAS_NEWOBJ); + optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_QMARKS; + + GenTree* cond = gtNewOperNode(GT_EQ, TYP_INT, hasValue, gtNewIconNode(0)); + GenTreeColon* colon = gtNewColonNode(TYP_REF, gtNewNull(), allocRoot); + GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); + + // QMARK has to be a top-level statement + const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); + impStoreToTemp(result, qmark, CHECK_SPILL_ALL); + lvaSetClass(result, typeToBox, true); + lvaSetClass(objLclNum, typeToBox, true); + impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); + return true; +} + //------------------------------------------------------------------------ // impImportAndPushBox: build and import a value-type box // @@ -3405,12 +3489,6 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("async box with call")); } - // Get get the expression to box from the stack. - GenTree* op1 = nullptr; - GenTree* op2 = nullptr; - StackEntry se = impPopStack(); - GenTree* exprToBox = se.val; - // Look at what helper we should use. CorInfoHelpFunc boxHelper = info.compCompHnd->getBoxHelper(pResolvedToken->hClass); @@ -3419,71 +3497,17 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) // // obj = nullable._hasValue == 0 ? null : (boxed underlying value) // - if ((boxHelper == CORINFO_HELP_BOX_NULLABLE) && opts.OptimizationEnabled() && !compCurBB->isRunRarely() && - !eeIsSharedInst(pResolvedToken->hClass)) + if ((boxHelper == CORINFO_HELP_BOX_NULLABLE) && impImportAndPushBoxForNullable(pResolvedToken)) { - CORINFO_CLASS_HANDLE typeToBox = info.compCompHnd->getTypeForBox(pResolvedToken->hClass); - - // We need to compose a new CORINFO_RESOLVED_TOKEN for the underlying (typeToBox) type - CORINFO_RESOLVED_TOKEN tk = *pResolvedToken; - tk.hClass = typeToBox; - tk.tokenType = CORINFO_TOKENKIND_Casting; - GenTree* obj = gtNewAllocObjNode(&tk, info.compMethodHnd, false); - - if (obj != nullptr) - { - // First, decompose the Nullable<> into _hasValue and _value fields. - // - GenTreeFlags indirFlags = GTF_EMPTY; - exprToBox = impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags); - CORINFO_FIELD_HANDLE valueFldHnd = info.compCompHnd->getFieldInClass(pResolvedToken->hClass, 1); - CORINFO_CLASS_HANDLE valueStructCls = NO_CLASS_HANDLE; - static_assert(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); - unsigned cnsValueOffset = info.compCompHnd->getFieldOffset(valueFldHnd); - ClassLayout* layout = nullptr; - CorInfoType corFldType = info.compCompHnd->getFieldType(valueFldHnd, &valueStructCls); - var_types valueType = TypeHandleToVarType(corFldType, valueStructCls, &layout); - GenTree* valueOffset = gtNewIconNode(cnsValueOffset, TYP_I_IMPL); - GenTree* valueAddr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(exprToBox), valueOffset); - GenTree* value = gtNewLoadValueNode(valueType, layout, valueAddr); - GenTree* hasValue = gtNewLoadValueNode(TYP_UBYTE, nullptr, gtCloneExpr(exprToBox)); - - // Now we need to copy value into the allocated box - // - unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); - GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); - GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); - GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); - GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); - GenTree* storeData = - gtNewStoreValueNode(valueType, layout, dataPtr, gtCloneExpr(value), GTF_IND_NONFAULTING); - - // Wrap it all in two commas, it will look like: - // lcl = allocobj - // *(lcl + sizeof(void*)) = value - // lcl - // - GenTreeOp* copyData = gtNewOperNode(GT_COMMA, TYP_REF, storeData, gtCloneExpr(objLcl)); - GenTreeOp* allocRoot = gtNewOperNode(GT_COMMA, TYP_REF, storeAlloc, copyData); - - // QMARK expansion will propagate block flags properly. - compCurBB->SetFlags(BBF_HAS_NEWOBJ); - optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_QMARKS; - - GenTree* cond = gtNewOperNode(GT_EQ, TYP_INT, hasValue, gtNewIconNode(0)); - GenTreeColon* colon = gtNewColonNode(TYP_REF, gtNewNull(), allocRoot); - GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); - - // QMARK has to be a top-level statement - const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); - impStoreToTemp(result, qmark, CHECK_SPILL_ALL); - lvaSetClass(result, typeToBox, true); - lvaSetClass(objLclNum, typeToBox, true); - impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); - return; - } + return; } + // Get get the expression to box from the stack. + GenTree* op1 = nullptr; + GenTree* op2 = nullptr; + StackEntry se = impPopStack(); + GenTree* exprToBox = se.val; + // Determine what expansion to prefer. // // In size/time/debuggable constrained modes, the helper call diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 048070d11b22c7..29f411707d8725 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14750,7 +14750,7 @@ PhaseStatus Compiler::fgExpandQmarkNodes(bool early) JITDUMP("Qmark expansion created new throw blocks\n"); } - return PhaseStatus::MODIFIED_EVERYTHING; + return compQmarkUsed ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; } //------------------------------------------------------------------------ From 179458e955a22f7dbcecb7f3128006bd730e317e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 5 Dec 2025 13:27:37 +0100 Subject: [PATCH 11/17] clean up --- src/coreclr/jit/importer.cpp | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 86833b043c1c16..796e9cd6339f60 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3356,7 +3356,10 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, } //------------------------------------------------------------------------ -// impImportAndPushBoxForNullable: import a "box Nullable<>" as an inlined sequence +// impImportAndPushBoxForNullable: import a "box Nullable<>" as an inlined sequence: +// arg._hasValue == 0 ? +// null : +// (lcl = allocobj; *(lcl + sizeof(void*)) = arg._value; lcl) // // Arguments: // pResolvedToken - resolved token from the box operation @@ -3389,21 +3392,19 @@ bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedT GenTree* exprToBox = impPopStack().val; - // First, decompose the Nullable<> into _hasValue and _value fields. + // Decompose the Nullable<> arg into _hasValue and _value fields. + // + GenTree* value; + GenTree* hasValue; + impLoadNullableFields(exprToBox, pResolvedToken->hClass, &hasValue, &value); + + // Calculate the type and layout of the 'value' field // - GenTreeFlags indirFlags = GTF_EMPTY; - exprToBox = impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags); - CORINFO_FIELD_HANDLE valueFldHnd = info.compCompHnd->getFieldInClass(pResolvedToken->hClass, 1); CORINFO_CLASS_HANDLE valueStructCls = NO_CLASS_HANDLE; - static_assert(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); - unsigned cnsValueOffset = info.compCompHnd->getFieldOffset(valueFldHnd); - ClassLayout* layout = nullptr; - CorInfoType corFldType = info.compCompHnd->getFieldType(valueFldHnd, &valueStructCls); - var_types valueType = TypeHandleToVarType(corFldType, valueStructCls, &layout); - GenTree* valueOffset = gtNewIconNode(cnsValueOffset, TYP_I_IMPL); - GenTree* valueAddr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(exprToBox), valueOffset); - GenTree* value = gtNewLoadValueNode(valueType, layout, valueAddr); - GenTree* hasValue = gtNewLoadValueNode(TYP_UBYTE, nullptr, gtCloneExpr(exprToBox)); + CORINFO_FIELD_HANDLE valueFldHnd = info.compCompHnd->getFieldInClass(pResolvedToken->hClass, 1); + CorInfoType corFldType = info.compCompHnd->getFieldType(valueFldHnd, &valueStructCls); + ClassLayout* layout = nullptr; + var_types valueType = TypeHandleToVarType(corFldType, valueStructCls, &layout); // Now we need to copy value into the allocated box // From 25d856adf4c04e13c90f91ef213ab784cba373cc Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 5 Dec 2025 13:42:23 +0100 Subject: [PATCH 12/17] clean up --- src/coreclr/jit/importer.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 796e9cd6339f60..809146fcab61fb 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3357,8 +3357,8 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, //------------------------------------------------------------------------ // impImportAndPushBoxForNullable: import a "box Nullable<>" as an inlined sequence: -// arg._hasValue == 0 ? -// null : +// arg._hasValue == 0 ? +// null : // (lcl = allocobj; *(lcl + sizeof(void*)) = arg._value; lcl) // // Arguments: From ab99de1b90028dfaf072bd03a6aa6c6a65de0483 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 5 Dec 2025 22:26:21 +0100 Subject: [PATCH 13/17] feedback --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/gentree.h | 12 +++++++++++ src/coreclr/jit/importer.cpp | 32 +++++++++++++++++------------ src/coreclr/jit/morph.cpp | 40 ++++++++++++++++++++++-------------- 4 files changed, 57 insertions(+), 29 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index dd0b8271a8731b..c8a016463b132f 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5565,7 +5565,7 @@ class Compiler Statement* fgNewStmtFromTree(GenTree* tree, const DebugInfo& di); GenTreeQmark* fgGetTopLevelQmark(GenTree* expr, GenTree** ppDst = nullptr); - bool fgExpandQmarkStmt(BasicBlock* block, Statement* stmt); + bool fgExpandQmarkStmt(BasicBlock* block, Statement* stmt, bool onlyEarlyQmarks); PhaseStatus fgExpandQmarkNodes(bool early); bool fgSimpleLowerCastOfSmpOp(LIR::Range& range, GenTreeCast* cast); diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index ca74f9db9c7ec0..b61320e86f32e4 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -502,6 +502,8 @@ enum GenTreeFlags : unsigned GTF_BOX_CLONED = 0x40000000, // GT_BOX -- this box and its operand has been cloned, cannot assume it to be single-use anymore GTF_BOX_VALUE = 0x80000000, // GT_BOX -- "box" is on a value type + GTF_QMARK_EARLY_EXPAND = 0x01000000, // GT_QMARK -- early expansion of the QMARK node is required + GTF_ARR_ADDR_NONNULL = 0x80000000, // GT_ARR_ADDR -- this array's address is not null #define HANDLE_KIND_INDEX_SHIFT 24 @@ -5937,6 +5939,16 @@ struct GenTreeQmark : public GenTreeOp gtThenLikelihood = thenLikelihood; } + bool IsEarlyExpandableQmark() const + { + return gtFlags & GTF_QMARK_EARLY_EXPAND; + } + + void SetEarlyExpandableQmark() + { + gtFlags |= GTF_QMARK_EARLY_EXPAND; + } + #if DEBUGGABLE_GENTREE GenTreeQmark() : GenTreeOp() diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 809146fcab61fb..dfcee32c955a97 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3392,19 +3392,22 @@ bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedT GenTree* exprToBox = impPopStack().val; - // Decompose the Nullable<> arg into _hasValue and _value fields. + // Decompose the Nullable<> arg into _hasValue and _value fields + // and calculate the type and layout of the 'value' field // - GenTree* value; - GenTree* hasValue; - impLoadNullableFields(exprToBox, pResolvedToken->hClass, &hasValue, &value); - - // Calculate the type and layout of the 'value' field - // - CORINFO_CLASS_HANDLE valueStructCls = NO_CLASS_HANDLE; + GenTreeFlags indirFlags = GTF_EMPTY; + exprToBox = impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags); CORINFO_FIELD_HANDLE valueFldHnd = info.compCompHnd->getFieldInClass(pResolvedToken->hClass, 1); - CorInfoType corFldType = info.compCompHnd->getFieldType(valueFldHnd, &valueStructCls); - ClassLayout* layout = nullptr; - var_types valueType = TypeHandleToVarType(corFldType, valueStructCls, &layout); + CORINFO_CLASS_HANDLE valueStructCls = NO_CLASS_HANDLE; + static_assert(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); + unsigned cnsValueOffset = info.compCompHnd->getFieldOffset(valueFldHnd); + ClassLayout* layout = nullptr; + CorInfoType corFldType = info.compCompHnd->getFieldType(valueFldHnd, &valueStructCls); + var_types valueType = TypeHandleToVarType(corFldType, valueStructCls, &layout); + GenTree* valueOffset = gtNewIconNode(cnsValueOffset, TYP_I_IMPL); + GenTree* valueAddr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(exprToBox), valueOffset); + GenTree* value = gtNewLoadValueNode(valueType, layout, valueAddr); + GenTree* hasValue = gtNewLoadValueNode(TYP_UBYTE, nullptr, gtCloneExpr(exprToBox)); // Now we need to copy value into the allocated box // @@ -3427,9 +3430,12 @@ bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedT compCurBB->SetFlags(BBF_HAS_NEWOBJ); optMethodFlags |= OMF_HAS_NEWOBJ | OMF_HAS_EARLY_QMARKS; - GenTree* cond = gtNewOperNode(GT_EQ, TYP_INT, hasValue, gtNewIconNode(0)); + GenTreeOp* cond = gtNewOperNode(GT_EQ, TYP_INT, hasValue, gtNewIconNode(0)); GenTreeColon* colon = gtNewColonNode(TYP_REF, gtNewNull(), allocRoot); - GenTree* qmark = gtNewQmarkNode(TYP_REF, cond, colon); + GenTreeQmark* qmark = gtNewQmarkNode(TYP_REF, cond, colon); + + // We have to expand early since GT_ALLOCOBJ must be a top-level statement + qmark->SetEarlyExpandableQmark(); // QMARK has to be a top-level statement const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNullableBox")); diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 29f411707d8725..7f843cbd69c951 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -14396,8 +14396,9 @@ GenTreeQmark* Compiler::fgGetTopLevelQmark(GenTree* expr, GenTree** ppDst /* = N // fgExpandQmarkStmt: expand a qmark into control flow // // Arguments: -// block - block containing the qmark -// stmt - statement containing the qmark +// block - block containing the qmark +// stmt - statement containing the qmark +// onlyEarlyQmarks - if true, only expand qmarks that are marked for early expansion // // Returns: // true if the expansion introduced a throwing block @@ -14452,7 +14453,7 @@ GenTreeQmark* Compiler::fgGetTopLevelQmark(GenTree* expr, GenTree** ppDst /* = N // If the qmark assigns to a variable, then create tmps for "then" // and "else" results and assign the temp to the variable as a writeback step. // -bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) +bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt, bool onlyEarlyQmarks) { bool introducedThrow = false; GenTree* expr = stmt->GetRootNode(); @@ -14465,6 +14466,11 @@ bool Compiler::fgExpandQmarkStmt(BasicBlock* block, Statement* stmt) return false; } + if (onlyEarlyQmarks && !qmark->IsEarlyExpandableQmark()) + { + return false; + } + #ifdef DEBUG if (verbose) { @@ -14721,25 +14727,29 @@ PhaseStatus Compiler::fgExpandQmarkNodes(bool early) return PhaseStatus::MODIFIED_NOTHING; } + if (!compQmarkUsed) + { + return PhaseStatus::MODIFIED_NOTHING; + } + bool introducedThrows = false; - if (compQmarkUsed) + for (BasicBlock* const block : Blocks()) { - for (BasicBlock* const block : Blocks()) + for (Statement* const stmt : block->Statements()) { - for (Statement* const stmt : block->Statements()) - { - GenTree* expr = stmt->GetRootNode(); -#ifdef DEBUG - fgPreExpandQmarkChecks(expr); -#endif - introducedThrows |= fgExpandQmarkStmt(block, stmt); - } + GenTree* expr = stmt->GetRootNode(); + INDEBUG(fgPreExpandQmarkChecks(expr)); + introducedThrows |= fgExpandQmarkStmt(block, stmt, early); } + } + #ifdef DEBUG + if (!early) + { fgPostExpandQmarkChecks(); -#endif } +#endif compQmarkRationalized = true; @@ -14750,7 +14760,7 @@ PhaseStatus Compiler::fgExpandQmarkNodes(bool early) JITDUMP("Qmark expansion created new throw blocks\n"); } - return compQmarkUsed ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING; + return PhaseStatus::MODIFIED_EVERYTHING; } //------------------------------------------------------------------------ From 69f6243873132d2a88918f5fc5de1de19ee80639 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 6 Dec 2025 02:30:37 +0100 Subject: [PATCH 14/17] Simplify the logic for now --- src/coreclr/jit/importer.cpp | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index dfcee32c955a97..d9d2aec9ba56a4 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3371,25 +3371,26 @@ bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedT { assert(info.compCompHnd->getBoxHelper(pResolvedToken->hClass) == CORINFO_HELP_BOX_NULLABLE); - if (opts.OptimizationDisabled() || compCurBB->isRunRarely() || eeIsSharedInst(pResolvedToken->hClass)) + if (opts.OptimizationDisabled() || compCurBB->isRunRarely()) { return false; } - CORINFO_CLASS_HANDLE typeToBox = info.compCompHnd->getTypeForBox(pResolvedToken->hClass); - - // We need to compose a new CORINFO_RESOLVED_TOKEN for the underlying (typeToBox) type - // E.g. if we are boxing Nullable, typeToBox is System.Int32. - CORINFO_RESOLVED_TOKEN tk = *pResolvedToken; - tk.hClass = typeToBox; - tk.tokenType = CORINFO_TOKENKIND_Casting; - GenTree* obj = gtNewAllocObjNode(&tk, info.compMethodHnd, false); - - if (obj == nullptr) + if (eeIsSharedInst(pResolvedToken->hClass) || IsReadyToRun()) { - return false; + // TODO-CQ: Enable the optimization for shared generics and R2R scenarios. + // The current machinery requires a ResolvedToken (basically, 'newobj underlyingType' + // that we don't have). + return true; } + CORINFO_CLASS_HANDLE typeToBox = info.compCompHnd->getTypeForBox(pResolvedToken->hClass); + + bool hasSideEffects; + CorInfoHelpFunc helperTemp = info.compCompHnd->getNewHelper(typeToBox, &hasSideEffects); + GenTreeAllocObj* allocObj = + gtNewAllocObjNode(helperTemp, hasSideEffects, typeToBox, TYP_REF, gtNewIconEmbClsHndNode(typeToBox)); + GenTree* exprToBox = impPopStack().val; // Decompose the Nullable<> arg into _hasValue and _value fields @@ -3412,8 +3413,8 @@ bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedT // Now we need to copy value into the allocated box // unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); - GenTree* storeAlloc = gtNewTempStore(objLclNum, obj); - GenTree* objLcl = gtNewLclvNode(objLclNum, genActualType(obj)); + GenTree* storeAlloc = gtNewTempStore(objLclNum, allocObj); + GenTree* objLcl = gtNewLclvNode(objLclNum, TYP_REF); GenTree* pOffset = gtNewIconNode(TARGET_POINTER_SIZE, TYP_I_IMPL); GenTreeOp* dataPtr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(objLcl), pOffset); GenTree* storeData = gtNewStoreValueNode(valueType, layout, dataPtr, gtCloneExpr(value), GTF_IND_NONFAULTING); From 426c2271bd192280dd3d9b1b02fa9b8397d61597 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 6 Dec 2025 02:38:24 +0100 Subject: [PATCH 15/17] clean up --- src/coreclr/jit/importer.cpp | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index d9d2aec9ba56a4..4d231ea19ed40b 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3369,14 +3369,15 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, // bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedToken) { - assert(info.compCompHnd->getBoxHelper(pResolvedToken->hClass) == CORINFO_HELP_BOX_NULLABLE); + CORINFO_CLASS_HANDLE nullableCls = pResolvedToken->hClass; + assert(info.compCompHnd->getBoxHelper(nullableCls) == CORINFO_HELP_BOX_NULLABLE); if (opts.OptimizationDisabled() || compCurBB->isRunRarely()) { return false; } - if (eeIsSharedInst(pResolvedToken->hClass) || IsReadyToRun()) + if (eeIsSharedInst(nullableCls) || IsReadyToRun()) { // TODO-CQ: Enable the optimization for shared generics and R2R scenarios. // The current machinery requires a ResolvedToken (basically, 'newobj underlyingType' @@ -3384,13 +3385,6 @@ bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedT return true; } - CORINFO_CLASS_HANDLE typeToBox = info.compCompHnd->getTypeForBox(pResolvedToken->hClass); - - bool hasSideEffects; - CorInfoHelpFunc helperTemp = info.compCompHnd->getNewHelper(typeToBox, &hasSideEffects); - GenTreeAllocObj* allocObj = - gtNewAllocObjNode(helperTemp, hasSideEffects, typeToBox, TYP_REF, gtNewIconEmbClsHndNode(typeToBox)); - GenTree* exprToBox = impPopStack().val; // Decompose the Nullable<> arg into _hasValue and _value fields @@ -3398,7 +3392,7 @@ bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedT // GenTreeFlags indirFlags = GTF_EMPTY; exprToBox = impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags); - CORINFO_FIELD_HANDLE valueFldHnd = info.compCompHnd->getFieldInClass(pResolvedToken->hClass, 1); + CORINFO_FIELD_HANDLE valueFldHnd = info.compCompHnd->getFieldInClass(nullableCls, 1); CORINFO_CLASS_HANDLE valueStructCls = NO_CLASS_HANDLE; static_assert(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); unsigned cnsValueOffset = info.compCompHnd->getFieldOffset(valueFldHnd); @@ -3410,6 +3404,14 @@ bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedT GenTree* value = gtNewLoadValueNode(valueType, layout, valueAddr); GenTree* hasValue = gtNewLoadValueNode(TYP_UBYTE, nullptr, gtCloneExpr(exprToBox)); + // Create the allocation node for the box + // + CORINFO_CLASS_HANDLE typeToBox = info.compCompHnd->getTypeForBox(nullableCls); + bool hasSideEffects; + CorInfoHelpFunc helperTemp = info.compCompHnd->getNewHelper(typeToBox, &hasSideEffects); + GenTree* typeToBoxHnd = gtNewIconEmbClsHndNode(typeToBox); + GenTreeAllocObj* allocObj = gtNewAllocObjNode(helperTemp, hasSideEffects, typeToBox, TYP_REF, typeToBoxHnd); + // Now we need to copy value into the allocated box // unsigned objLclNum = lvaGrabTemp(true DEBUGARG("obj nullable box")); @@ -3444,6 +3446,10 @@ bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedT lvaSetClass(result, typeToBox, true); lvaSetClass(objLclNum, typeToBox, true); impPushOnStack(gtNewLclvNode(result, TYP_REF), typeInfo(typeToBox)); + + JITDUMP(" inlined BOX(%s) as QMARK allocating box and copying fields:\n", eeGetClassName(nullableCls)); + DISPTREE(qmark); + JITDUMP("\n"); return true; } From b2987b3c0377e82616bb594a969db4cb34ae11bb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 6 Dec 2025 14:55:05 +0100 Subject: [PATCH 16/17] fix assert --- src/coreclr/jit/importer.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 4d231ea19ed40b..19da48100b9147 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3385,13 +3385,16 @@ bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedT return true; } - GenTree* exprToBox = impPopStack().val; + GenTree* nullableObj = impPopStack().val; // Decompose the Nullable<> arg into _hasValue and _value fields // and calculate the type and layout of the 'value' field // - GenTreeFlags indirFlags = GTF_EMPTY; - exprToBox = impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags); + GenTreeFlags indirFlags = GTF_EMPTY; + nullableObj = impGetNodeAddr(nullableObj, CHECK_SPILL_ALL, &indirFlags); + GenTree* nullableObjClone; + nullableObj = impCloneExpr(nullableObj, &nullableObjClone, CHECK_SPILL_ALL, nullptr DEBUGARG("nullable obj clone")); + CORINFO_FIELD_HANDLE valueFldHnd = info.compCompHnd->getFieldInClass(nullableCls, 1); CORINFO_CLASS_HANDLE valueStructCls = NO_CLASS_HANDLE; static_assert(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); @@ -3400,9 +3403,9 @@ bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedT CorInfoType corFldType = info.compCompHnd->getFieldType(valueFldHnd, &valueStructCls); var_types valueType = TypeHandleToVarType(corFldType, valueStructCls, &layout); GenTree* valueOffset = gtNewIconNode(cnsValueOffset, TYP_I_IMPL); - GenTree* valueAddr = gtNewOperNode(GT_ADD, TYP_BYREF, gtCloneExpr(exprToBox), valueOffset); + GenTree* valueAddr = gtNewOperNode(GT_ADD, TYP_BYREF, nullableObjClone, valueOffset); GenTree* value = gtNewLoadValueNode(valueType, layout, valueAddr); - GenTree* hasValue = gtNewLoadValueNode(TYP_UBYTE, nullptr, gtCloneExpr(exprToBox)); + GenTree* hasValue = gtNewLoadValueNode(TYP_UBYTE, nullptr, nullableObj); // Create the allocation node for the box // From ca5eead8fb68d432b33b3cdad1c68d783cc75b73 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 6 Dec 2025 14:56:19 +0100 Subject: [PATCH 17/17] fix typo --- 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 19da48100b9147..749309e8d7c09b 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3382,7 +3382,7 @@ bool Compiler::impImportAndPushBoxForNullable(CORINFO_RESOLVED_TOKEN* pResolvedT // TODO-CQ: Enable the optimization for shared generics and R2R scenarios. // The current machinery requires a ResolvedToken (basically, 'newobj underlyingType' // that we don't have). - return true; + return false; } GenTree* nullableObj = impPopStack().val;