From a16dc86bdf665bee3ff945289c2826d294e280b2 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 26 Jan 2024 10:49:31 +0100 Subject: [PATCH 1/2] JIT: Improve consistency around indir flags; directly copy flags in `gtCloneExpr` - Switch `GT_STORE_DYN_BLK` to be on the same plan as other indirs; add a `gtNewStoreDynBlkNode` and use `gtInitializeIndirNode` to set its flags correctly. Add support for it to `GenTree::SetIndirExceptionFlags`. Remove conservative flag setting from its constructor. - Fix propagation of `GTF_EXCEPT` from `GT_CMPXCHG`'s data source inside `GenTree::SetIndirExceptionFlags` - Set flags directly from the source node in `gtCloneExpr` Fix #97524 --- src/coreclr/jit/compiler.h | 3 +++ src/coreclr/jit/gentree.cpp | 40 +++++++++++++++++++++++++++++++++--- src/coreclr/jit/gentree.h | 5 +---- src/coreclr/jit/importer.cpp | 3 +-- 4 files changed, 42 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index ee779d375939d3..593392f5fd9e12 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3341,6 +3341,9 @@ class Compiler GenTreeBlk* gtNewStoreBlkNode( ClassLayout* layout, GenTree* addr, GenTree* data, GenTreeFlags indirFlags = GTF_EMPTY); + GenTreeStoreDynBlk* gtNewStoreDynBlkNode( + GenTree* addr, GenTree* data, GenTree* dynamicSize, GenTreeFlags indirFlags = GTF_EMPTY); + GenTreeStoreInd* gtNewStoreIndNode( var_types type, GenTree* addr, GenTree* data, GenTreeFlags indirFlags = GTF_EMPTY); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 4f907b26fd16d2..20ee5cd0e94cb1 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -8603,6 +8603,34 @@ GenTreeBlk* Compiler::gtNewStoreBlkNode(ClassLayout* layout, GenTree* addr, GenT return store; } +//------------------------------------------------------------------------------ +// gtNewStoreDynBlkNode : Create a dynamic block store node. +// +// Arguments: +// addr - Destionation address +// data - Value to store (init val or indirection representing a location) +// +// indirFlags - Indirection flags +// +// Return Value: +// The created GT_STORE_BLK node. +// +GenTreeStoreDynBlk* Compiler::gtNewStoreDynBlkNode(GenTree* addr, + GenTree* data, + GenTree* dynamicSize, + GenTreeFlags indirFlags) +{ + assert((indirFlags & GTF_IND_INVARIANT) == 0); + assert(data->IsInitVal() || data->OperIs(GT_IND)); + + GenTreeStoreDynBlk* store = new (this, GT_STORE_DYN_BLK) GenTreeStoreDynBlk(addr, data, dynamicSize); + store->gtFlags |= GTF_ASG; + gtInitializeIndirNode(store, indirFlags); + gtInitializeStoreNode(store, data); + + return store; +} + //------------------------------------------------------------------------------ // gtNewStoreIndNode : Create an indirect store node. // @@ -9663,7 +9691,7 @@ GenTree* Compiler::gtCloneExpr(GenTree* tree) assert(copy->gtOper == oper); copy->gtVNPair = tree->gtVNPair; // A cloned tree gets the original's Value number pair - copy->gtFlags |= tree->gtFlags; + copy->gtFlags = tree->gtFlags; #if defined(DEBUG) // Non-node debug flags should be propagated from 'tree' to 'copy' @@ -10706,7 +10734,7 @@ bool GenTree::Precedes(GenTree* other) // void GenTree::SetIndirExceptionFlags(Compiler* comp) { - assert(OperIsIndirOrArrMetaData() && (OperIsSimple() || OperIs(GT_CMPXCHG))); + assert(OperIsIndirOrArrMetaData() && (OperIsSimple() || OperIs(GT_CMPXCHG, GT_STORE_DYN_BLK))); if (IndirMayFault(comp)) { @@ -10723,10 +10751,16 @@ void GenTree::SetIndirExceptionFlags(Compiler* comp) { gtFlags |= gtGetOp2()->gtFlags & GTF_EXCEPT; } - if (OperIs(GT_CMPXCHG)) + else if (OperIs(GT_CMPXCHG)) { + gtFlags |= AsCmpXchg()->Data()->gtFlags & GTF_EXCEPT; gtFlags |= AsCmpXchg()->Comparand()->gtFlags & GTF_EXCEPT; } + else if (OperIs(GT_STORE_DYN_BLK)) + { + gtFlags |= AsStoreDynBlk()->Data()->gtFlags & GTF_EXCEPT; + gtFlags |= AsStoreDynBlk()->gtDynamicSize->gtFlags & GTF_EXCEPT; + } } #ifdef DEBUG diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index ef4dd9be146911..0af319f9c39fed 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -7481,10 +7481,7 @@ struct GenTreeStoreDynBlk : public GenTreeBlk GenTreeStoreDynBlk(GenTree* dstAddr, GenTree* data, GenTree* dynamicSize) : GenTreeBlk(GT_STORE_DYN_BLK, TYP_VOID, dstAddr, data, nullptr), gtDynamicSize(dynamicSize) { - // Conservatively the 'dstAddr' could be null or point into the global heap. - // Likewise, this is a store and so must be marked with the GTF_ASG flag. - gtFlags |= (GTF_ASG | GTF_EXCEPT | GTF_GLOB_REF); - gtFlags |= (dynamicSize->gtFlags & GTF_ALL_EFFECT); + gtFlags |= dynamicSize->gtFlags & GTF_ALL_EFFECT; } #if DEBUGGABLE_GENTREE diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 5393c4a7425421..ebbc5415d5e24c 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -10296,8 +10296,7 @@ void Compiler::impImportBlockCode(BasicBlock* block) op3 = gtNewCastNode(TYP_I_IMPL, op3, /* fromUnsigned */ true, TYP_I_IMPL); #endif - op1 = new (this, GT_STORE_DYN_BLK) GenTreeStoreDynBlk(op1, op2, op3); - op1->gtFlags |= indirFlags; + op1 = gtNewStoreDynBlkNode(op1, op2, op3, indirFlags); } goto SPILL_APPEND; } From 981711d07f087e350ec207a86c4ad6dcf5e01ce1 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 26 Jan 2024 11:12:54 +0100 Subject: [PATCH 2/2] Fix function header --- src/coreclr/jit/gentree.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 20ee5cd0e94cb1..0148896f670b9c 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -8583,7 +8583,7 @@ GenTree* Compiler::gtNewLoadValueNode(var_types type, ClassLayout* layout, GenTr // // Arguments: // layout - The struct layout -// addr - Destionation address +// addr - Destination address // data - Value to store // indirFlags - Indirection flags // @@ -8607,13 +8607,13 @@ GenTreeBlk* Compiler::gtNewStoreBlkNode(ClassLayout* layout, GenTree* addr, GenT // gtNewStoreDynBlkNode : Create a dynamic block store node. // // Arguments: -// addr - Destionation address -// data - Value to store (init val or indirection representing a location) -// -// indirFlags - Indirection flags +// addr - Destination address +// data - Value to store (init val or indirection representing a location) +// dynamicSize - Node that computes number of bytes to store +// indirFlags - Indirection flags // // Return Value: -// The created GT_STORE_BLK node. +// The created GT_STORE_DYN_BLK node. // GenTreeStoreDynBlk* Compiler::gtNewStoreDynBlkNode(GenTree* addr, GenTree* data,