Skip to content

Commit

Permalink
JIT: Change GTF_ICON_INITCLASS -> GTF_IND_INITCLASS (#85396)
Browse files Browse the repository at this point in the history
The JIT has a flag GTF_ICON_INITCLASS that represents that accesses off
that address are cctor dependent. Hoisting uses this to avoid hoisting
cctor dependent indirections unless all cctors are also hoisted.
However, local constant prop/VN-based constant prop do not handle this
flag, so we could run into cases where addresses with GTF_ICON_INITCLASS
were propagated and then subsequently hoisted incorrectly.

This change moves the flag to an OperIsIndir() flag instead of being a
flag on the constant. After some digging, I found that the original
reason the flag was not an indir flag was simply that there were no more
indir flags available, but we do have available flags today. This fix
is much simpler than the alternatives which would be to teach VN/local
copy prop to propagate this GTF_ICON_INITCLASS flag.

Also remove GTF_FLD_INITCLASS which is never set.
  • Loading branch information
jakobbotsch authored Apr 26, 2023
1 parent 678b4c8 commit 0bd15cb
Show file tree
Hide file tree
Showing 6 changed files with 26 additions and 44 deletions.
4 changes: 4 additions & 0 deletions src/coreclr/jit/compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9766,6 +9766,10 @@ void cTreeFlags(Compiler* comp, GenTree* tree)
{
chars += printf("[IND_NONNULL]");
}
if (tree->gtFlags & GTF_IND_INITCLASS)
{
chars += printf("[IND_INITCLASS]");
}
break;

case GT_MUL:
Expand Down
22 changes: 9 additions & 13 deletions src/coreclr/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10594,6 +10594,12 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, _In_ _In_opt_
--msgLength;
break;
}
if (tree->gtFlags & GTF_IND_INITCLASS)
{
printf("I");
--msgLength;
break;
}
if (tree->gtFlags & GTF_IND_INVARIANT)
{
printf("#");
Expand Down Expand Up @@ -10762,19 +10768,9 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, _In_ _In_opt_
case GT_CNS_INT:
if (tree->IsIconHandle())
{
if ((tree->gtFlags & GTF_ICON_INITCLASS) != 0)
{
printf("I"); // Static Field handle with INITCLASS requirement
--msgLength;
break;
}
else
{
// Some other handle
printf("H");
--msgLength;
break;
}
printf("H");
--msgLength;
break;
}
goto DASH;

Expand Down
11 changes: 2 additions & 9 deletions src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,6 @@ enum GenTreeFlags : unsigned int

GTF_FLD_TLS = 0x80000000, // GT_FIELD_ADDR -- field address is a Windows x86 TLS reference
GTF_FLD_VOLATILE = 0x40000000, // GT_FIELD -- same as GTF_IND_VOLATILE
GTF_FLD_INITCLASS = 0x20000000, // GT_FIELD/GT_FIELD_ADDR -- field access requires preceding class/static init helper
GTF_FLD_TGT_HEAP = 0x10000000, // GT_FIELD -- same as GTF_IND_TGT_HEAP

GTF_INX_RNGCHK = 0x80000000, // GT_INDEX_ADDR -- this array address should be range-checked
Expand All @@ -494,9 +493,10 @@ enum GenTreeFlags : unsigned int
GTF_IND_UNALIGNED = 0x02000000, // OperIsIndir() -- the load or store is unaligned (we assume worst case alignment of 1 byte)
GTF_IND_INVARIANT = 0x01000000, // GT_IND -- the target is invariant (a prejit indirection)
GTF_IND_NONNULL = 0x00400000, // GT_IND -- the indirection never returns null (zero)
GTF_IND_INITCLASS = 0x00200000, // OperIsIndir() -- the indirection requires preceding static cctor

GTF_IND_FLAGS = GTF_IND_VOLATILE | GTF_IND_NONFAULTING | GTF_IND_UNALIGNED | GTF_IND_INVARIANT |
GTF_IND_NONNULL | GTF_IND_TGT_NOT_HEAP | GTF_IND_TGT_HEAP,
GTF_IND_NONNULL | GTF_IND_TGT_NOT_HEAP | GTF_IND_TGT_HEAP | GTF_IND_INITCLASS,

GTF_ADDRMODE_NO_CSE = 0x80000000, // GT_ADD/GT_MUL/GT_LSH -- Do not CSE this node only, forms complex
// addressing mode
Expand Down Expand Up @@ -541,13 +541,6 @@ enum GenTreeFlags : unsigned int

// GTF_ICON_REUSE_REG_VAL = 0x00800000 // GT_CNS_INT -- GTF_REUSE_REG_VAL, defined above
GTF_ICON_SIMD_COUNT = 0x00200000, // GT_CNS_INT -- constant is Vector<T>.Count
GTF_ICON_INITCLASS = 0x00100000, // GT_CNS_INT -- Constant is used to access a static that requires preceding
// class/static init helper. In some cases, the constant is
// the address of the static field itself, and in other cases
// there's an extra layer of indirection and it is the address
// of the cell that the runtime will fill in with the address
// of the static field; in both of those cases, the constant
// is what gets flagged.

GTF_OVERFLOW = 0x10000000, // Supported for: GT_ADD, GT_SUB, GT_MUL and GT_CAST.
// Requires an overflow check. Use gtOverflow(Ex)() to check this flag.
Expand Down
18 changes: 10 additions & 8 deletions src/coreclr/jit/importer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4167,10 +4167,11 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT
outerFldSeq = nullptr;
}

bool isHoistable = false;
bool isStaticReadOnlyInitedRef = false;
unsigned typeIndex = 0;
GenTree* op1;
bool isHoistable = false;
bool isStaticReadOnlyInitedRef = false;
GenTreeFlags indirFlags = GTF_EMPTY;
unsigned typeIndex = 0;
GenTree* op1;
switch (pFieldInfo->fieldAccessor)
{
case CORINFO_FIELD_STATIC_GENERICS_STATIC_HELPER:
Expand Down Expand Up @@ -4348,16 +4349,17 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT
INDEBUG(op1->AsIntCon()->gtTargetHandle = reinterpret_cast<size_t>(pResolvedToken->hField));
if (pFieldInfo->fieldFlags & CORINFO_FLG_FIELD_INITCLASS)
{
op1->gtFlags |= GTF_ICON_INITCLASS;
indirFlags |= GTF_IND_INITCLASS;
}
break;
}
}

if (isBoxedStatic)
{
op1 = gtNewIndir(TYP_REF, op1, GTF_IND_INVARIANT | GTF_IND_NONFAULTING | GTF_IND_NONNULL);
op1 = gtNewOperNode(GT_ADD, TYP_BYREF, op1, gtNewIconNode(TARGET_POINTER_SIZE, outerFldSeq));
op1 = gtNewIndir(TYP_REF, op1, GTF_IND_INVARIANT | GTF_IND_NONFAULTING | GTF_IND_NONNULL | indirFlags);
indirFlags = GTF_EMPTY;
op1 = gtNewOperNode(GT_ADD, TYP_BYREF, op1, gtNewIconNode(TARGET_POINTER_SIZE, outerFldSeq));
}

if (!(access & CORINFO_ACCESS_ADDRESS))
Expand All @@ -4366,7 +4368,7 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT
lclTyp = TypeHandleToVarType(pFieldInfo->fieldType, pFieldInfo->structType, &layout);

// TODO-CQ: mark the indirections non-faulting.
op1 = (lclTyp == TYP_STRUCT) ? gtNewBlkIndir(layout, op1) : gtNewIndir(lclTyp, op1);
op1 = (lclTyp == TYP_STRUCT) ? gtNewBlkIndir(layout, op1, indirFlags) : gtNewIndir(lclTyp, op1, indirFlags);

if (isStaticReadOnlyInitedRef)
{
Expand Down
7 changes: 0 additions & 7 deletions src/coreclr/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5347,13 +5347,6 @@ GenTree* Compiler::fgMorphExpandTlsFieldAddr(GenTree* tree)
// Mark this ICON as a TLS_HDL, codegen will use FS:[cns]
GenTree* tlsRef = gtNewIconHandleNode(WIN32_TLS_SLOTS, GTF_ICON_TLS_HDL);

// Translate GTF_FLD_INITCLASS to GTF_ICON_INITCLASS
if ((tree->gtFlags & GTF_FLD_INITCLASS) != 0)
{
tree->gtFlags &= ~GTF_FLD_INITCLASS;
tlsRef->gtFlags |= GTF_ICON_INITCLASS;
}

tlsRef = gtNewIndir(TYP_I_IMPL, tlsRef, GTF_IND_NONFAULTING | GTF_IND_INVARIANT);

if (dllRef != nullptr)
Expand Down
8 changes: 1 addition & 7 deletions src/coreclr/jit/optimizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7537,13 +7537,7 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
return fgWalkResult::WALK_CONTINUE;
}

// Initclass CLS_VARs and IconHandles are the base cases of cctor dependent trees.
// In the IconHandle case, it's of course the dereference, rather than the constant itself, that is
// truly dependent on the cctor. So a more precise approach would be to separately propagate
// isCctorDependent and isAddressWhoseDereferenceWouldBeCctorDependent, but we don't for
// simplicity/throughput; the constant itself would be considered non-hoistable anyway, since
// optIsCSEcandidate returns false for constants.
bool treeIsCctorDependent = tree->OperIs(GT_CNS_INT) && ((tree->gtFlags & GTF_ICON_INITCLASS) != 0);
bool treeIsCctorDependent = tree->OperIsIndir() && ((tree->gtFlags & GTF_IND_INITCLASS) != 0);
bool treeIsInvariant = true;
bool treeHasHoistableChildren = false;
int childCount;
Expand Down

0 comments on commit 0bd15cb

Please sign in to comment.