Skip to content

Commit

Permalink
JIT: Remove GTF_IND_INVARIANT and GTF_IND_NONFAULTING flags check…
Browse files Browse the repository at this point in the history 
…ing (dotnet#104976)

These flags are strictly optimizations. Having them required to be set for
certain indirs based on context of the operand introduces IR invariants that are
annoying to work with since it suddenly becomes necessary for all
transformations to consider "did we just introduce this IR shape?". Instead,
handle these patterns during morph as the optimization it is and remove the
strict flags checking from `fgDebugCheckFlags`.

Also fix `HandleKindDataIsInvariant` which returned true in some questionable
cases, and remove some unnecessary indir flags that morph was setting for
`GTF_ICON_OBJ_HDL`.
  • Loading branch information
@jakobbotsch
jakobbotsch authored Jul 18, 2024
1 parent 1f0b156 commit 49a2a55
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 54 deletions.
31 changes: 0 additions & 31 deletions src/coreclr/jit/fgdiagnostic.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3360,37 +3360,6 @@ void Compiler::fgDebugCheckFlags(GenTree* tree, BasicBlock* block)
break;

case GT_IND:
// Do we have a constant integer address as op1 that is also a handle?
if (op1->IsIconHandle())
{
if ((tree->gtFlags & GTF_IND_INVARIANT) != 0)
{
actualFlags |= GTF_IND_INVARIANT;
}
if ((tree->gtFlags & GTF_IND_NONFAULTING) != 0)
{
actualFlags |= GTF_IND_NONFAULTING;
}

GenTreeFlags handleKind = op1->GetIconHandleFlag();

// Some of these aren't handles to invariant data...
if (GenTree::HandleKindDataIsInvariant(handleKind) && (handleKind != GTF_ICON_FTN_ADDR))
{
expectedFlags |= GTF_IND_INVARIANT;
}
else
{
// For statics, we expect the GTF_GLOB_REF to be set. However, we currently
// fail to set it in a number of situations, and so this check is disabled.
// TODO: enable checking of GTF_GLOB_REF.
// expectedFlags |= GTF_GLOB_REF;
}

// Currently we expect all indirections with constant addresses to be nonfaulting.
expectedFlags |= GTF_IND_NONFAULTING;
}

assert(((tree->gtFlags & GTF_IND_TGT_NOT_HEAP) == 0) || ((tree->gtFlags & GTF_IND_TGT_HEAP) == 0));
break;

Expand Down
36 changes: 27 additions & 9 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7516,8 +7516,6 @@ GenTree* Compiler::gtNewIndOfIconHandleNode(var_types indType, size_t addr, GenT

if (isInvariant)
{
assert(GenTree::HandleKindDataIsInvariant(iconFlags));

// This indirection also is invariant.
indirFlags |= GTF_IND_INVARIANT;

Expand Down Expand Up @@ -10576,8 +10574,7 @@ void GenTree::SetIndirExceptionFlags(Compiler* comp)

//------------------------------------------------------------------------------
// HandleKindDataIsInvariant: Returns true if the data referred to by a handle
// address is guaranteed to be invariant. Note that GTF_ICON_FTN_ADDR handles may
// or may not point to invariant data.
// address is guaranteed to be invariant.
//
// Arguments:
// flags - GenTree flags for handle.
Expand All @@ -10588,11 +10585,32 @@ bool GenTree::HandleKindDataIsInvariant(GenTreeFlags flags)
GenTreeFlags handleKind = flags & GTF_ICON_HDL_MASK;
assert(handleKind != GTF_EMPTY);

// All handle types are assumed invariant except those specifically listed here.

return (handleKind != GTF_ICON_STATIC_HDL) && // Pointer to a mutable class Static variable
(handleKind != GTF_ICON_BBC_PTR) && // Pointer to a mutable basic block count value
(handleKind != GTF_ICON_GLOBAL_PTR); // Pointer to mutable data from the VM state
switch (handleKind)
{
case GTF_ICON_SCOPE_HDL:
case GTF_ICON_CLASS_HDL:
case GTF_ICON_METHOD_HDL:
case GTF_ICON_FIELD_HDL:
case GTF_ICON_STR_HDL:
case GTF_ICON_CONST_PTR:
case GTF_ICON_VARG_HDL:
case GTF_ICON_PINVKI_HDL:
case GTF_ICON_TOKEN_HDL:
case GTF_ICON_TLS_HDL:
case GTF_ICON_CIDMID_HDL:
case GTF_ICON_FIELD_SEQ:
case GTF_ICON_STATIC_ADDR_PTR:
case GTF_ICON_SECREL_OFFSET:
case GTF_ICON_TLSGD_OFFSET:
return true;
case GTF_ICON_FTN_ADDR:
case GTF_ICON_GLOBAL_PTR:
case GTF_ICON_STATIC_HDL:
case GTF_ICON_BBC_PTR:
case GTF_ICON_STATIC_BOX_PTR:
default:
return false;
}
}

#ifdef DEBUG
Expand Down
24 changes: 12 additions & 12 deletions src/coreclr/jit/gentree.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -513,18 +513,18 @@ enum GenTreeFlags : unsigned int
GTF_ICON_FIELD_HDL = 0x04000000, // GT_CNS_INT -- constant is a field handle
GTF_ICON_STATIC_HDL = 0x05000000, // GT_CNS_INT -- constant is a handle to static data
GTF_ICON_STR_HDL = 0x06000000, // GT_CNS_INT -- constant is a pinned handle pointing to a string object
GTF_ICON_OBJ_HDL = 0x12000000, // GT_CNS_INT -- constant is an object handle (e.g. frozen string or Type object)
GTF_ICON_CONST_PTR = 0x07000000, // GT_CNS_INT -- constant is a pointer to immutable data, (e.g. IAT_PPVALUE)
GTF_ICON_GLOBAL_PTR = 0x08000000, // GT_CNS_INT -- constant is a pointer to mutable data (e.g. from the VM state)
GTF_ICON_VARG_HDL = 0x09000000, // GT_CNS_INT -- constant is a var arg cookie handle
GTF_ICON_PINVKI_HDL = 0x0A000000, // GT_CNS_INT -- constant is a pinvoke calli handle
GTF_ICON_TOKEN_HDL = 0x0B000000, // GT_CNS_INT -- constant is a token handle (other than class, method or field)
GTF_ICON_TLS_HDL = 0x0C000000, // GT_CNS_INT -- constant is a TLS ref with offset
GTF_ICON_FTN_ADDR = 0x0D000000, // GT_CNS_INT -- constant is a function address
GTF_ICON_CIDMID_HDL = 0x0E000000, // GT_CNS_INT -- constant is a class ID or a module ID
GTF_ICON_BBC_PTR = 0x0F000000, // GT_CNS_INT -- constant is a basic block count pointer
GTF_ICON_STATIC_BOX_PTR = 0x10000000, // GT_CNS_INT -- constant is an address of the box for a STATIC_IN_HEAP field
GTF_ICON_FIELD_SEQ = 0x11000000, // <--------> -- constant is a FieldSeq* (used only as VNHandle)
GTF_ICON_OBJ_HDL = 0x07000000, // GT_CNS_INT -- constant is an object handle (e.g. frozen string or Type object)
GTF_ICON_CONST_PTR = 0x08000000, // GT_CNS_INT -- constant is a pointer to immutable data, (e.g. IAT_PPVALUE)
GTF_ICON_GLOBAL_PTR = 0x09000000, // GT_CNS_INT -- constant is a pointer to mutable data (e.g. from the VM state)
GTF_ICON_VARG_HDL = 0x0A000000, // GT_CNS_INT -- constant is a var arg cookie handle
GTF_ICON_PINVKI_HDL = 0x0B000000, // GT_CNS_INT -- constant is a pinvoke calli handle
GTF_ICON_TOKEN_HDL = 0x0C000000, // GT_CNS_INT -- constant is a token handle (other than class, method or field)
GTF_ICON_TLS_HDL = 0x0D000000, // GT_CNS_INT -- constant is a TLS ref with offset
GTF_ICON_FTN_ADDR = 0x0E000000, // GT_CNS_INT -- constant is a function address
GTF_ICON_CIDMID_HDL = 0x0F000000, // GT_CNS_INT -- constant is a class ID or a module ID
GTF_ICON_BBC_PTR = 0x10000000, // GT_CNS_INT -- constant is a basic block count pointer
GTF_ICON_STATIC_BOX_PTR = 0x11000000, // GT_CNS_INT -- constant is an address of the box for a STATIC_IN_HEAP field
GTF_ICON_FIELD_SEQ = 0x12000000, // <--------> -- constant is a FieldSeq* (used only as VNHandle)
GTF_ICON_STATIC_ADDR_PTR = 0x13000000, // GT_CNS_INT -- constant is a pointer to a static base address
GTF_ICON_SECREL_OFFSET = 0x14000000, // GT_CNS_INT -- constant is an offset in a certain section.
GTF_ICON_TLSGD_OFFSET = 0x15000000, // GT_CNS_INT -- constant is an argument to tls_get_addr.
Expand Down
12 changes: 10 additions & 2 deletions src/coreclr/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8679,9 +8679,17 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA

case GT_IND:
{
if (op1->IsIconHandle(GTF_ICON_OBJ_HDL))
if (op1->IsIconHandle())
{
tree->gtFlags |= (GTF_IND_INVARIANT | GTF_IND_NONFAULTING | GTF_IND_NONNULL);
// All indirections with (handle) constant addresses are
// nonfaulting.
tree->gtFlags |= GTF_IND_NONFAULTING;

// We know some handle types always point to invariant data.
if (GenTree::HandleKindDataIsInvariant(op1->GetIconHandleFlag()))
{
tree->gtFlags |= GTF_IND_INVARIANT;
}
}

GenTree* optimizedTree = fgMorphFinalizeIndir(tree->AsIndir());
Expand Down

0 comments on commit 49a2a55

Please sign in to comment.