Skip to content

Commit

Permalink
Delete a zero-diff quirk and update INDEX-related comments (#70398)
Browse files Browse the repository at this point in the history 
* Delete a zero-diff quirk

* Also update some comments

Removing GT_INDEX/ADDR(IND) references.
  • Loading branch information
@SingleAccretion
SingleAccretion committed Jun 8, 2022
1 parent 352c1df commit c0e9075
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 76 deletions.
9 changes: 0 additions & 9 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15714,15 +15714,6 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
PushSideEffects(node);
return Compiler::WALK_SKIP_SUBTREES;
}

// TODO-ADDR: remove this quirk added to avoid diffs.
if (node->OperIs(GT_IND) && node->AsIndir()->Addr()->OperIs(GT_INDEX_ADDR) &&
!m_compiler->fgGlobalMorph)
{
JITDUMP("Keep the GT_INDEX_ADDR and GT_IND together:\n");
PushSideEffects(node);
return Compiler::WALK_SKIP_SUBTREES;
}
}

// Generally all GT_CALL nodes are considered to have side-effects.
Expand Down
8 changes: 4 additions & 4 deletions src/coreclr/jit/gentree.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6414,10 +6414,10 @@ struct GenTreeBoundsChk : public GenTreeOp
BasicBlock* gtIndRngFailBB; // Basic block to jump to for index-out-of-range
SpecialCodeKind gtThrowKind; // Kind of throw block to branch to on failure

// Store some information about the array element type that was in the GT_INDEX node before morphing.
// Note that this information is also stored in the ARR_ADDR node of the morphed tree, but that can
// be hard to find.
var_types gtInxType; // Save the GT_INDEX type
// Store some information about the array element type that was in the GT_INDEX_ADDR node before morphing.
// Note that this information is also stored in the ARR_ADDR node of the morphed tree, but that can be hard
// to find.
var_types gtInxType; // The array element type.

GenTreeBoundsChk(GenTree* index, GenTree* length, SpecialCodeKind kind)
: GenTreeOp(GT_BOUNDS_CHECK, TYP_VOID, index, length)
Expand Down
126 changes: 65 additions & 61 deletions src/coreclr/jit/loopcloning.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2418,7 +2418,7 @@ bool Compiler::optExtractArrIndex(GenTree* tree, ArrIndex* result, unsigned lhsN
result->useBlock = compCurBB;
result->rank++;

// If the array element type (saved from the GT_INDEX node during morphing) is anything but
// If the array element type (saved from the GT_INDEX_ADDR node during morphing) is anything but
// TYP_REF, then it must the final level of jagged array.
assert(arrBndsChk->gtInxType != TYP_VOID);
*topLevelIsFinal = (arrBndsChk->gtInxType != TYP_REF);
Expand Down Expand Up @@ -2484,7 +2484,7 @@ bool Compiler::optReconstructArrIndexHelp(GenTree* tree, ArrIndex* result, unsig
//
// Arguments:
// tree the tree to be checked if it is an array [][][] operation.
// result OUT: the extracted GT_INDEX information.
// result OUT: the extracted GT_INDEX_ADDR information.
//
// Return Value:
// Returns true if array index can be extracted, else, return false. "rank" field in
Expand All @@ -2499,99 +2499,103 @@ bool Compiler::optReconstructArrIndexHelp(GenTree* tree, ArrIndex* result, unsig
// Note that the array expression is implied by the array bounds check under the COMMA, and the array bounds
// checks is what is parsed from the morphed tree; the array addressing expression is not parsed.
// However, the array bounds checks are not quite sufficient because of the way "morph" alters the trees.
// Specifically, we normally see a COMMA node with a LHS of the morphed array INDEX expression and RHS
// Specifically, we normally see a COMMA node with a LHS of the morphed array INDEX_ADDR expression and RHS
// of the bounds check. E.g., for int[][], a[i][j] we have a pre-morph tree:
//
// \--* INDEX int
// +--* INDEX ref
// | +--* LCL_VAR ref V00 loc0
// | \--* LCL_VAR int V02 loc2
// \--* LCL_VAR int V03 loc3
// \--* IND int
// \--* INDEX_ADDR byref int[]
// +--* IND ref
// | \--* INDEX_ADDR byref ref[]
// | +--* LCL_VAR ref V00 arg0
// | \--* LCL_VAR int V01 arg1
// \--* LCL_VAR int V02 arg2
//
// and post-morph tree:
//
// \--* COMMA int
// +--* ASG ref
// | +--* LCL_VAR ref V19 tmp12
// | +--* LCL_VAR ref V04 tmp1
// | \--* COMMA ref
// | +--* BOUNDS_CHECK_Rng void
// | | +--* LCL_VAR int V02 loc2
// | | +--* LCL_VAR int V01 arg1
// | | \--* ARR_LENGTH int
// | | \--* LCL_VAR ref V00 loc0
// | | \--* LCL_VAR ref V00 arg0
// | \--* IND ref
// | \--* ADD byref
// | +--* LCL_VAR ref V00 loc0
// | \--* ADD long
// | +--* LSH long
// | | +--* CAST long <- uint
// | | | \--* LCL_VAR int V02 loc2
// | | \--* CNS_INT long 3
// | \--* CNS_INT long 16 Fseq[#FirstElem]
// | \--* ARR_ADDR byref ref[]
// | \--* ADD byref
// | +--* LCL_VAR ref V00 arg0
// | \--* ADD long
// | +--* LSH long
// | | +--* CAST long <- uint
// | | | \--* LCL_VAR int V01 arg1
// | | \--* CNS_INT long 3
// | \--* CNS_INT long 16
// \--* COMMA int
// +--* BOUNDS_CHECK_Rng void
// | +--* LCL_VAR int V03 loc3
// | +--* LCL_VAR int V02 arg2
// | \--* ARR_LENGTH int
// | \--* LCL_VAR ref V19 tmp12
// | \--* LCL_VAR ref V04 tmp1
// \--* IND int
// \--* ADD byref
// +--* LCL_VAR ref V19 tmp12
// \--* ADD long
// +--* LSH long
// | +--* CAST long <- uint
// | | \--* LCL_VAR int V03 loc3
// | \--* CNS_INT long 2
// \--* CNS_INT long 16 Fseq[#FirstElem]
// \--* ARR_ADDR byref int[]
// \--* ADD byref
// +--* LCL_VAR ref V04 tmp1
// \--* ADD long
// +--* LSH long
// | +--* CAST long <- uint
// | | \--* LCL_VAR int V02 arg2
// | \--* CNS_INT long 2
// \--* CNS_INT long 16
//
// However, for an array of structs that contains an array field, e.g. ValueTuple<int[], int>[], expression
// a[i].Item1[j],
//
// \--* INDEX int
// +--* FIELD ref Item1
// | \--* ADDR byref
// | \--* INDEX struct<System.ValueTuple`2[System.Int32[],System.Int32], 16>
// | +--* LCL_VAR ref V01 loc1
// | \--* LCL_VAR int V04 loc4
// \--* LCL_VAR int V06 loc6
// \--* IND int
// \--* INDEX_ADDR byref int[]
// +--* FIELD ref Item1
// | \--* INDEX_ADDR byref System.ValueTuple`2[System.Int32[],System.Int32][]
// | +--* LCL_VAR ref V00 arg0
// | \--* LCL_VAR int V01 arg1
// \--* LCL_VAR int V02 arg2
//
// Morph "hoists" the bounds check above the struct field access:
//
// \--* COMMA int
// +--* ASG ref
// | +--* LCL_VAR ref V23 tmp16
// | +--* LCL_VAR ref V04 tmp1
// | \--* COMMA ref
// | +--* BOUNDS_CHECK_Rng void
// | | +--* LCL_VAR int V04 loc4
// | | +--* LCL_VAR int V01 arg1
// | | \--* ARR_LENGTH int
// | | \--* LCL_VAR ref V01 loc1
// | | \--* LCL_VAR ref V00 arg0
// | \--* IND ref
// | \--* ADDR byref Zero Fseq[Item1]
// | \--* IND struct<System.ValueTuple`2[System.Int32[],System.Int32], 16>
// | \--* ADD byref
// | +--* LCL_VAR ref V01 loc1
// | \--* ADD long
// | +--* LSH long
// | | +--* CAST long <- uint
// | | | \--* LCL_VAR int V04 loc4
// | | \--* CNS_INT long 4
// | \--* CNS_INT long 16 Fseq[#FirstElem]
// | \--* ARR_ADDR byref System.ValueTuple`2[System.Int32[],System.Int32][] Zero Fseq[Item1]
// | \--* ADD byref
// | +--* LCL_VAR ref V00 arg0
// | \--* ADD long
// | +--* LSH long
// | | +--* CAST long <- uint
// | | | \--* LCL_VAR int V01 arg1
// | | \--* CNS_INT long 4
// | \--* CNS_INT long 16
// \--* COMMA int
// +--* BOUNDS_CHECK_Rng void
// | +--* LCL_VAR int V06 loc6
// | +--* LCL_VAR int V02 arg2
// | \--* ARR_LENGTH int
// | \--* LCL_VAR ref V23 tmp16
// | \--* LCL_VAR ref V04 tmp1
// \--* IND int
// \--* ADD byref
// +--* LCL_VAR ref V23 tmp16
// \--* ADD long
// +--* LSH long
// | +--* CAST long <- uint
// | | \--* LCL_VAR int V06 loc6
// | \--* CNS_INT long 2
// \--* CNS_INT long 16 Fseq[#FirstElem]
// \--* ARR_ADDR byref int[]
// \--* ADD byref
// +--* LCL_VAR ref V04 tmp1
// \--* ADD long
// +--* LSH long
// | +--* CAST long <- uint
// | | \--* LCL_VAR int V02 arg2
// | \--* CNS_INT long 2
// \--* CNS_INT long 16
//
// This should not be parsed as a jagged array (e.g., a[i][j]). To ensure that it is not, the type of the
// GT_INDEX node is stashed in the GT_BOUNDS_CHECK node during morph. If we see a bounds check node where
// the GT_INDEX was not TYP_REF, then it must be the outermost jagged array level. E.g., if it is
// GT_INDEX_ADDR node is stashed in the GT_BOUNDS_CHECK node during morph. If we see a bounds check node
// where the GT_INDEX_ADDR was not TYP_REF, then it must be the outermost jagged array level. E.g., if it is
// TYP_STRUCT, then we have an array of structs, and any further bounds checks must be of one of its fields.
//
// It would be much better if we didn't need to parse these trees at all, and did all this work pre-morph.
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4785,7 +4785,7 @@ GenTree* Compiler::fgMorphIndexAddr(GenTreeIndexAddr* indexAddr)
addr = gtNewOperNode(GT_ADD, TYP_BYREF, arrRef, addr);
}

// TODO-Throughout: bash the INDEX_ADDR to ARR_ADDR here instead of creating a new node.
// TODO-Throughput: bash the INDEX_ADDR to ARR_ADDR here instead of creating a new node.
addr = new (this, GT_ARR_ADDR) GenTreeArrAddr(addr, elemTyp, elemStructType, elemOffs);

if (indexAddr->IsNotNull())
Expand Down Expand Up @@ -4825,7 +4825,7 @@ GenTree* Compiler::fgMorphIndexAddr(GenTreeIndexAddr* indexAddr)
tree = fgMorphTree(tree);
DBEXEC(tree == indexAddr, tree->gtDebugFlags &= ~GTF_DEBUG_NODE_MORPHED);

JITDUMP("fgMorphArrayIndex (after remorph):\n")
JITDUMP("fgMorphIndexAddr (after remorph):\n")
DISPTREE(tree)

return tree;
Expand Down

0 comments on commit c0e9075

Please sign in to comment.