Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete a zero-diff quirk and update INDEX-related comments #70398

Merged
merged 2 commits into from
Jun 8, 2022
Merged

Delete a zero-diff quirk and update INDEX-related comments #70398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bdaf206
Delete a zero-diff quirk
SingleAccretion Jun 7, 2022
eb8528d
Also update some comments
SingleAccretion Jun 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 0 additions & 9 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2294,7 +2294,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 @@ -2360,7 +2360,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 @@ -2375,99 +2375,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
Open in desktop
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