From 741dbd222179e9adbb3de474fce9d98ce7fd8560 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 10 Dec 2025 11:27:38 +0000 Subject: [PATCH 1/8] Initial plan From 98548f2bd3c4be9583240ae48dfe76aa897f51fa Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 10 Dec 2025 11:56:14 +0000 Subject: [PATCH 2/8] Fix impGetNodeAddr flag handling in all 5 locations Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com> --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/importer.cpp | 49 +++++++++++++++++++++++------------- 2 files changed, 32 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index abd9666e5114bf..4cb6395e34c643 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4855,7 +4855,7 @@ class Compiler bool impIsLegalRetBuf(GenTree* retBuf, GenTreeCall* call); GenTree* impStoreStructPtr(GenTree* destAddr, GenTree* value, unsigned curLevel, GenTreeFlags indirFlags = GTF_EMPTY); - GenTree* impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* pDerefFlags); + GenTree* impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* pDerefFlags, bool allowVolatileUnaligned = true); var_types impNormStructType(CORINFO_CLASS_HANDLE structHnd, var_types* simdBaseType = nullptr); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index e9e4bc20ee5931..426a0f9cc55b2f 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -805,9 +805,9 @@ GenTree* Compiler::impStoreStruct(GenTree* store, WellKnownArg wellKnownArgType = srcCall->ShouldHaveRetBufArg() ? WellKnownArg::RetBuffer : WellKnownArg::None; - // TODO-Bug?: verify if flags matter here + // Return buffers cannot be volatile or unaligned GenTreeFlags indirFlags = GTF_EMPTY; - GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, &indirFlags); + GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, &indirFlags, /* allowVolatileUnaligned */ false); if (!impIsLegalRetBuf(destAddr, srcCall)) { @@ -929,9 +929,9 @@ GenTree* Compiler::impStoreStruct(GenTree* store, if (call->ShouldHaveRetBufArg()) { // insert the return value buffer into the argument list as first byref parameter after 'this' - // TODO-Bug?: verify if flags matter here + // Return buffers cannot be volatile or unaligned GenTreeFlags indirFlags = GTF_EMPTY; - GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, &indirFlags); + GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, &indirFlags, /* allowVolatileUnaligned */ false); if (!impIsLegalRetBuf(destAddr, call)) { @@ -1096,18 +1096,20 @@ GenTree* Compiler::impStoreStructPtr(GenTree* destAddr, GenTree* value, unsigned // impGetNodeAddr: Get the address of a value. // // Arguments: -// val - The value in question -// curLevel - Stack level for spilling -// pDerefFlags - Flags to be used on dereference, nullptr when -// the address won't be dereferenced. Returned flags -// are included in the GTF_IND_COPYABLE_FLAGS mask. +// val - The value in question +// curLevel - Stack level for spilling +// pDerefFlags - Flags to be used on dereference, nullptr when +// the address won't be dereferenced. Returned flags +// are included in the GTF_IND_COPYABLE_FLAGS mask. +// allowVolatileUnaligned - If false, volatile or unaligned addresses will +// cause the value to be copied to a temporary // // Return Value: // In case "val" represents a location (is an indirection/local), // will return its address. Otherwise, address of a temporary assigned // the value of "val" will be returned. // -GenTree* Compiler::impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* pDerefFlags) +GenTree* Compiler::impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* pDerefFlags, bool allowVolatileUnaligned) { if (pDerefFlags != nullptr) { @@ -1121,7 +1123,15 @@ GenTree* Compiler::impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* case GT_STORE_BLK: if (pDerefFlags != nullptr) { - *pDerefFlags = val->gtFlags & GTF_IND_COPYABLE_FLAGS; + GenTreeFlags flags = val->gtFlags & GTF_IND_COPYABLE_FLAGS; + + // If volatile or unaligned is not allowed and present, fall through to create a temp + if (!allowVolatileUnaligned && ((flags & (GTF_IND_VOLATILE | GTF_IND_UNALIGNED)) != 0)) + { + break; + } + + *pDerefFlags = flags; return val->AsIndir()->Addr(); } break; @@ -3290,12 +3300,12 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, GenTree* objToBox = impPopStack().val; // Spill struct to get its address (to access hasValue field) - // TODO-Bug?: verify if flags matter here + // Transfer any volatile/unaligned flags to the indirection GenTreeFlags indirFlags = GTF_EMPTY; objToBox = impGetNodeAddr(objToBox, CHECK_SPILL_ALL, &indirFlags); static_assert(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); - impPushOnStack(gtNewIndir(TYP_UBYTE, objToBox), typeInfo(TYP_INT)); + impPushOnStack(gtNewIndir(TYP_UBYTE, objToBox, indirFlags), typeInfo(TYP_INT)); JITDUMP("\n Importing BOX; ISINST; BR_TRUE/FALSE as nullableVT.hasValue\n"); return returnToken; @@ -3675,9 +3685,9 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) return; } - // TODO-Bug?: verify if flags matter here + // Boxing helpers cannot accept volatile or unaligned addresses GenTreeFlags indirFlags = GTF_EMPTY; - op1 = gtNewHelperCallNode(boxHelper, TYP_REF, op2, impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags)); + op1 = gtNewHelperCallNode(boxHelper, TYP_REF, op2, impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags, /* allowVolatileUnaligned */ false)); } /* Push the result back on the stack, */ @@ -9537,9 +9547,12 @@ void Compiler::impImportBlockCode(BasicBlock* block) BADCODE3("Unexpected opcode (has to be LDFLD)", ": %02X", (int)opcode); } - // TODO-Bug?: verify if flags matter here - GenTreeFlags indirFlags = GTF_EMPTY; - obj = impGetNodeAddr(obj, CHECK_SPILL_ALL, &indirFlags); + // Get the address and any flags from the original access (volatile, unaligned, etc.) + GenTreeFlags objAddrFlags = GTF_EMPTY; + obj = impGetNodeAddr(obj, CHECK_SPILL_ALL, &objAddrFlags); + + // Combine the flags from the object address with any prefix flags + indirFlags |= objAddrFlags; } op1 = gtNewFieldAddrNode(resolvedToken.hField, obj, fieldInfo.offset); From 4be7539dffb531b0e2dd2fae1f26196dbbab23e7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 10 Dec 2025 12:08:47 +0000 Subject: [PATCH 3/8] Address code review feedback: remove extra blank line Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com> --- 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 426a0f9cc55b2f..ea920982affbf2 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -9550,7 +9550,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) // Get the address and any flags from the original access (volatile, unaligned, etc.) GenTreeFlags objAddrFlags = GTF_EMPTY; obj = impGetNodeAddr(obj, CHECK_SPILL_ALL, &objAddrFlags); - + // Combine the flags from the object address with any prefix flags indirFlags |= objAddrFlags; } From 270cf271c03ab2e0ae9812cac40c61bb3db397e3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 10 Dec 2025 12:11:11 +0000 Subject: [PATCH 4/8] Fix recursive call and optimize flag checking Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com> --- src/coreclr/jit/importer.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index ea920982affbf2..2de2872e690cb9 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -1123,15 +1123,13 @@ GenTree* Compiler::impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* case GT_STORE_BLK: if (pDerefFlags != nullptr) { - GenTreeFlags flags = val->gtFlags & GTF_IND_COPYABLE_FLAGS; - // If volatile or unaligned is not allowed and present, fall through to create a temp - if (!allowVolatileUnaligned && ((flags & (GTF_IND_VOLATILE | GTF_IND_UNALIGNED)) != 0)) + if (!allowVolatileUnaligned && ((val->gtFlags & (GTF_IND_VOLATILE | GTF_IND_UNALIGNED)) != 0)) { break; } - - *pDerefFlags = flags; + + *pDerefFlags = val->gtFlags & GTF_IND_COPYABLE_FLAGS; return val->AsIndir()->Addr(); } break; @@ -1148,7 +1146,7 @@ GenTree* Compiler::impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* case GT_COMMA: impAppendTree(val->AsOp()->gtGetOp1(), curLevel, impCurStmtDI); - return impGetNodeAddr(val->AsOp()->gtGetOp2(), curLevel, pDerefFlags); + return impGetNodeAddr(val->AsOp()->gtGetOp2(), curLevel, pDerefFlags, allowVolatileUnaligned); default: break; From 7c2aedda685941e0a685c69e701890de54465df2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 10 Dec 2025 12:41:12 +0000 Subject: [PATCH 5/8] Rename allowVolatileUnaligned to allowConfiguredDerefs and check GTF_IND_INITCLASS Co-authored-by: jakobbotsch <7887810+jakobbotsch@users.noreply.github.com> --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/importer.cpp | 34 +++++++++++++++++----------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 4cb6395e34c643..81ca302ccee550 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4855,7 +4855,7 @@ class Compiler bool impIsLegalRetBuf(GenTree* retBuf, GenTreeCall* call); GenTree* impStoreStructPtr(GenTree* destAddr, GenTree* value, unsigned curLevel, GenTreeFlags indirFlags = GTF_EMPTY); - GenTree* impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* pDerefFlags, bool allowVolatileUnaligned = true); + GenTree* impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* pDerefFlags, bool allowConfiguredDerefs = true); var_types impNormStructType(CORINFO_CLASS_HANDLE structHnd, var_types* simdBaseType = nullptr); diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 2de2872e690cb9..31ac1738af540b 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -805,9 +805,9 @@ GenTree* Compiler::impStoreStruct(GenTree* store, WellKnownArg wellKnownArgType = srcCall->ShouldHaveRetBufArg() ? WellKnownArg::RetBuffer : WellKnownArg::None; - // Return buffers cannot be volatile or unaligned + // Return buffers cannot have volatile, unaligned, or initclass flags GenTreeFlags indirFlags = GTF_EMPTY; - GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, &indirFlags, /* allowVolatileUnaligned */ false); + GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, &indirFlags, /* allowConfiguredDerefs */ false); if (!impIsLegalRetBuf(destAddr, srcCall)) { @@ -929,9 +929,9 @@ GenTree* Compiler::impStoreStruct(GenTree* store, if (call->ShouldHaveRetBufArg()) { // insert the return value buffer into the argument list as first byref parameter after 'this' - // Return buffers cannot be volatile or unaligned + // Return buffers cannot have volatile, unaligned, or initclass flags GenTreeFlags indirFlags = GTF_EMPTY; - GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, &indirFlags, /* allowVolatileUnaligned */ false); + GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, &indirFlags, /* allowConfiguredDerefs */ false); if (!impIsLegalRetBuf(destAddr, call)) { @@ -1096,20 +1096,20 @@ GenTree* Compiler::impStoreStructPtr(GenTree* destAddr, GenTree* value, unsigned // impGetNodeAddr: Get the address of a value. // // Arguments: -// val - The value in question -// curLevel - Stack level for spilling -// pDerefFlags - Flags to be used on dereference, nullptr when -// the address won't be dereferenced. Returned flags -// are included in the GTF_IND_COPYABLE_FLAGS mask. -// allowVolatileUnaligned - If false, volatile or unaligned addresses will -// cause the value to be copied to a temporary +// val - The value in question +// curLevel - Stack level for spilling +// pDerefFlags - Flags to be used on dereference, nullptr when +// the address won't be dereferenced. Returned flags +// are included in the GTF_IND_COPYABLE_FLAGS mask. +// allowConfiguredDerefs - If false, derefs with volatile, unaligned, or initclass +// flags will cause the value to be copied to a temporary // // Return Value: // In case "val" represents a location (is an indirection/local), // will return its address. Otherwise, address of a temporary assigned // the value of "val" will be returned. // -GenTree* Compiler::impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* pDerefFlags, bool allowVolatileUnaligned) +GenTree* Compiler::impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* pDerefFlags, bool allowConfiguredDerefs) { if (pDerefFlags != nullptr) { @@ -1123,8 +1123,8 @@ GenTree* Compiler::impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* case GT_STORE_BLK: if (pDerefFlags != nullptr) { - // If volatile or unaligned is not allowed and present, fall through to create a temp - if (!allowVolatileUnaligned && ((val->gtFlags & (GTF_IND_VOLATILE | GTF_IND_UNALIGNED)) != 0)) + // If configured derefs (volatile, unaligned, initclass) are not allowed and present, fall through to create a temp + if (!allowConfiguredDerefs && ((val->gtFlags & (GTF_IND_VOLATILE | GTF_IND_UNALIGNED | GTF_IND_INITCLASS)) != 0)) { break; } @@ -1146,7 +1146,7 @@ GenTree* Compiler::impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* case GT_COMMA: impAppendTree(val->AsOp()->gtGetOp1(), curLevel, impCurStmtDI); - return impGetNodeAddr(val->AsOp()->gtGetOp2(), curLevel, pDerefFlags, allowVolatileUnaligned); + return impGetNodeAddr(val->AsOp()->gtGetOp2(), curLevel, pDerefFlags, allowConfiguredDerefs); default: break; @@ -3683,9 +3683,9 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) return; } - // Boxing helpers cannot accept volatile or unaligned addresses + // Boxing helpers cannot accept addresses with volatile, unaligned, or initclass flags GenTreeFlags indirFlags = GTF_EMPTY; - op1 = gtNewHelperCallNode(boxHelper, TYP_REF, op2, impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags, /* allowVolatileUnaligned */ false)); + op1 = gtNewHelperCallNode(boxHelper, TYP_REF, op2, impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags, /* allowConfiguredDerefs */ false)); } /* Push the result back on the stack, */ From ca6c9f254c157a4624709bc0e6128d69ea5479dd Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 11 Dec 2025 14:43:56 +0100 Subject: [PATCH 6/8] Fix bugs, slightly refactor --- src/coreclr/jit/compiler.h | 2 +- src/coreclr/jit/gentree.h | 7 +++- src/coreclr/jit/importer.cpp | 69 ++++++++++++++++--------------- src/coreclr/jit/importercalls.cpp | 2 +- 4 files changed, 43 insertions(+), 37 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 81ca302ccee550..531da99573c961 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4855,7 +4855,7 @@ class Compiler bool impIsLegalRetBuf(GenTree* retBuf, GenTreeCall* call); GenTree* impStoreStructPtr(GenTree* destAddr, GenTree* value, unsigned curLevel, GenTreeFlags indirFlags = GTF_EMPTY); - GenTree* impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* pDerefFlags, bool allowConfiguredDerefs = true); + GenTree* impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags allowedMustPreserveIndirFlags, GenTreeFlags* pIndirFlags); var_types impNormStructType(CORINFO_CLASS_HANDLE structHnd, var_types* simdBaseType = nullptr); diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 0726b9b9f8846f..69edce3db643ea 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -485,7 +485,12 @@ enum GenTreeFlags : unsigned GTF_IND_INITCLASS = 0x00200000, // OperIsIndir() -- the indirection requires preceding static cctor GTF_IND_ALLOW_NON_ATOMIC = 0x00100000, // GT_IND -- this memory access does not need to be atomic - GTF_IND_COPYABLE_FLAGS = GTF_IND_VOLATILE | GTF_IND_NONFAULTING | GTF_IND_UNALIGNED | GTF_IND_INITCLASS, + // Represents flags that an indirection based on another indirection must preserve + GTF_IND_MUST_PRESERVE_FLAGS = GTF_IND_VOLATILE | GTF_IND_UNALIGNED | GTF_IND_INITCLASS, + + // Represents flags that an indirection based on another indirection can and must preserve + GTF_IND_COPYABLE_FLAGS = GTF_IND_MUST_PRESERVE_FLAGS | GTF_IND_NONFAULTING, + GTF_IND_FLAGS = GTF_IND_COPYABLE_FLAGS | GTF_IND_NONNULL | GTF_IND_TGT_NOT_HEAP | GTF_IND_TGT_HEAP | GTF_IND_INVARIANT | GTF_IND_ALLOW_NON_ATOMIC, GTF_ADDRMODE_NO_CSE = 0x80000000, // GT_ADD/GT_MUL/GT_LSH -- Do not CSE this node only, forms complex diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 31ac1738af540b..486f016129933d 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -807,11 +807,12 @@ GenTree* Compiler::impStoreStruct(GenTree* store, // Return buffers cannot have volatile, unaligned, or initclass flags GenTreeFlags indirFlags = GTF_EMPTY; - GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, &indirFlags, /* allowConfiguredDerefs */ false); + GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, GTF_IND_MUST_PRESERVE_FLAGS, &indirFlags); - if (!impIsLegalRetBuf(destAddr, srcCall)) + if (((indirFlags & GTF_IND_MUST_PRESERVE_FLAGS) != GTF_EMPTY) || !impIsLegalRetBuf(destAddr, srcCall)) { - unsigned tmp = lvaGrabTemp(false DEBUGARG("stack copy for value returned via return buffer")); + unsigned tmp = lvaGrabTemp(false DEBUGARG( + printfAlloc("stack copy for value returned via return buffer for flags %x", (unsigned)indirFlags))); lvaSetStruct(tmp, srcCall->gtRetClsHnd, false); GenTree* spilledCall = gtNewStoreLclVarNode(tmp, srcCall); @@ -931,11 +932,12 @@ GenTree* Compiler::impStoreStruct(GenTree* store, // insert the return value buffer into the argument list as first byref parameter after 'this' // Return buffers cannot have volatile, unaligned, or initclass flags GenTreeFlags indirFlags = GTF_EMPTY; - GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, &indirFlags, /* allowConfiguredDerefs */ false); + GenTree* destAddr = impGetNodeAddr(store, CHECK_SPILL_ALL, GTF_IND_MUST_PRESERVE_FLAGS, &indirFlags); - if (!impIsLegalRetBuf(destAddr, call)) + if (((indirFlags & GTF_IND_MUST_PRESERVE_FLAGS) != GTF_EMPTY) || !impIsLegalRetBuf(destAddr, call)) { - unsigned tmp = lvaGrabTemp(false DEBUGARG("stack copy for value returned via return buffer")); + unsigned tmp = lvaGrabTemp(false DEBUGARG( + printfAlloc("stack copy for value returned via return buffer for flags %x", (unsigned)indirFlags))); lvaSetStruct(tmp, call->gtRetClsHnd, false); destAddr = gtNewLclVarAddrNode(tmp, TYP_I_IMPL); @@ -1096,43 +1098,38 @@ GenTree* Compiler::impStoreStructPtr(GenTree* destAddr, GenTree* value, unsigned // impGetNodeAddr: Get the address of a value. // // Arguments: -// val - The value in question -// curLevel - Stack level for spilling -// pDerefFlags - Flags to be used on dereference, nullptr when -// the address won't be dereferenced. Returned flags -// are included in the GTF_IND_COPYABLE_FLAGS mask. -// allowConfiguredDerefs - If false, derefs with volatile, unaligned, or initclass -// flags will cause the value to be copied to a temporary +// val - The value in question +// curLevel - Stack level for spilling +// allowedMustPreserveIndirFlags - If 'val' is an indirection and it has any must-preserve indir flags (like +// volatile), then those flags must be included in this mask to be allowed +// through without creating a temp. +// pIndirFlags - [out] Flags that indirs created based on this address can and should set. +// Returned flags are included in the GTF_IND_COPYABLE_FLAGS mask. // // Return Value: // In case "val" represents a location (is an indirection/local), // will return its address. Otherwise, address of a temporary assigned // the value of "val" will be returned. // -GenTree* Compiler::impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* pDerefFlags, bool allowConfiguredDerefs) +GenTree* Compiler::impGetNodeAddr(GenTree* val, + unsigned curLevel, + GenTreeFlags allowedMustPreserveIndirFlags, + GenTreeFlags* pIndirFlags) { - if (pDerefFlags != nullptr) - { - *pDerefFlags = GTF_EMPTY; - } + *pIndirFlags = GTF_EMPTY; switch (val->OperGet()) { case GT_BLK: case GT_IND: case GT_STOREIND: case GT_STORE_BLK: - if (pDerefFlags != nullptr) + if (((val->gtFlags & GTF_IND_MUST_PRESERVE_FLAGS & ~allowedMustPreserveIndirFlags) != 0)) { - // If configured derefs (volatile, unaligned, initclass) are not allowed and present, fall through to create a temp - if (!allowConfiguredDerefs && ((val->gtFlags & (GTF_IND_VOLATILE | GTF_IND_UNALIGNED | GTF_IND_INITCLASS)) != 0)) - { - break; - } - - *pDerefFlags = val->gtFlags & GTF_IND_COPYABLE_FLAGS; - return val->AsIndir()->Addr(); + break; } - break; + + *pIndirFlags = val->gtFlags & GTF_IND_COPYABLE_FLAGS; + return val->AsIndir()->Addr(); case GT_LCL_VAR: case GT_STORE_LCL_VAR: @@ -1146,12 +1143,13 @@ GenTree* Compiler::impGetNodeAddr(GenTree* val, unsigned curLevel, GenTreeFlags* case GT_COMMA: impAppendTree(val->AsOp()->gtGetOp1(), curLevel, impCurStmtDI); - return impGetNodeAddr(val->AsOp()->gtGetOp2(), curLevel, pDerefFlags, allowConfiguredDerefs); + return impGetNodeAddr(val->AsOp()->gtGetOp2(), curLevel, allowedMustPreserveIndirFlags, pIndirFlags); default: break; } + assert(!val->OperIsStore()); unsigned lclNum = lvaGrabTemp(true DEBUGARG("location for address-of(RValue)")); impStoreToTemp(lclNum, val, curLevel); @@ -3300,7 +3298,8 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, // Spill struct to get its address (to access hasValue field) // Transfer any volatile/unaligned flags to the indirection GenTreeFlags indirFlags = GTF_EMPTY; - objToBox = impGetNodeAddr(objToBox, CHECK_SPILL_ALL, &indirFlags); + objToBox = + impGetNodeAddr(objToBox, CHECK_SPILL_ALL, GTF_IND_MUST_PRESERVE_FLAGS, &indirFlags); static_assert(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); impPushOnStack(gtNewIndir(TYP_UBYTE, objToBox, indirFlags), typeInfo(TYP_INT)); @@ -3685,7 +3684,8 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) // Boxing helpers cannot accept addresses with volatile, unaligned, or initclass flags GenTreeFlags indirFlags = GTF_EMPTY; - op1 = gtNewHelperCallNode(boxHelper, TYP_REF, op2, impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, &indirFlags, /* allowConfiguredDerefs */ false)); + op1 = gtNewHelperCallNode(boxHelper, TYP_REF, op2, + impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, GTF_IND_INITCLASS, &indirFlags)); } /* Push the result back on the stack, */ @@ -8534,7 +8534,8 @@ void Compiler::impImportBlockCode(BasicBlock* block) } else { - op1 = impGetNodeAddr(op1, CHECK_SPILL_ALL, nullptr); + GenTreeFlags indirFlags; + op1 = impGetNodeAddr(op1, CHECK_SPILL_ALL, GTF_EMPTY, &indirFlags); } JITDUMP("\n ... optimized to ...\n"); @@ -9547,7 +9548,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) // Get the address and any flags from the original access (volatile, unaligned, etc.) GenTreeFlags objAddrFlags = GTF_EMPTY; - obj = impGetNodeAddr(obj, CHECK_SPILL_ALL, &objAddrFlags); + obj = impGetNodeAddr(obj, CHECK_SPILL_ALL, GTF_IND_MUST_PRESERVE_FLAGS, &objAddrFlags); // Combine the flags from the object address with any prefix flags indirFlags |= objAddrFlags; @@ -10360,7 +10361,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) // Get the address of the refany GenTreeFlags indirFlags = GTF_EMPTY; - op1 = impGetNodeAddr(op1, CHECK_SPILL_ALL, &indirFlags); + op1 = impGetNodeAddr(op1, CHECK_SPILL_ALL, GTF_IND_MUST_PRESERVE_FLAGS, &indirFlags); // Fetch the type from the correct slot op1 = gtNewOperNode(GT_ADD, TYP_BYREF, op1, diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index 22478354a3ab8d..9aea393e693c00 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -5357,7 +5357,7 @@ GenTree* Compiler::impSRCSUnsafeIntrinsic(NamedIntrinsic intrinsic, } else { - addr = impGetNodeAddr(op1, CHECK_SPILL_ALL, &indirFlags); + addr = impGetNodeAddr(op1, CHECK_SPILL_ALL, GTF_IND_MUST_PRESERVE_FLAGS, &indirFlags); } if (info.compCompHnd->getClassAlignmentRequirement(fromTypeHnd) < From 0595a5d9b8d85acd1bb20330f6196d82efafd813 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 12 Dec 2025 20:27:09 +0100 Subject: [PATCH 7/8] Apply suggestion from @jakobbotsch --- 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 486f016129933d..8f90edeea16f2f 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3682,7 +3682,7 @@ void Compiler::impImportAndPushBox(CORINFO_RESOLVED_TOKEN* pResolvedToken) return; } - // Boxing helpers cannot accept addresses with volatile, unaligned, or initclass flags + // Boxing helpers allow the "initclass" indir flag, but not volatile/unaligned flags GenTreeFlags indirFlags = GTF_EMPTY; op1 = gtNewHelperCallNode(boxHelper, TYP_REF, op2, impGetNodeAddr(exprToBox, CHECK_SPILL_ALL, GTF_IND_INITCLASS, &indirFlags)); From 3391737914532f06143cfc1fe58c7151ea80f6e6 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 12 Dec 2025 21:06:40 +0100 Subject: [PATCH 8/8] Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- 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 8f90edeea16f2f..8315d46e24397e 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -812,7 +812,7 @@ GenTree* Compiler::impStoreStruct(GenTree* store, if (((indirFlags & GTF_IND_MUST_PRESERVE_FLAGS) != GTF_EMPTY) || !impIsLegalRetBuf(destAddr, srcCall)) { unsigned tmp = lvaGrabTemp(false DEBUGARG( - printfAlloc("stack copy for value returned via return buffer for flags %x", (unsigned)indirFlags))); + printfAlloc("stack copy for value returned via return buffer"))); lvaSetStruct(tmp, srcCall->gtRetClsHnd, false); GenTree* spilledCall = gtNewStoreLclVarNode(tmp, srcCall); @@ -937,7 +937,7 @@ GenTree* Compiler::impStoreStruct(GenTree* store, if (((indirFlags & GTF_IND_MUST_PRESERVE_FLAGS) != GTF_EMPTY) || !impIsLegalRetBuf(destAddr, call)) { unsigned tmp = lvaGrabTemp(false DEBUGARG( - printfAlloc("stack copy for value returned via return buffer for flags %x", (unsigned)indirFlags))); + printfAlloc("stack copy for value returned via return buffer"))); lvaSetStruct(tmp, call->gtRetClsHnd, false); destAddr = gtNewLclVarAddrNode(tmp, TYP_I_IMPL);