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

Jump to bottom

[JIT] Fix re-use val zero on GC tracking #84051

Merged
merged 3 commits into from
Apr 4, 2023
Merged

[JIT] Fix re-use val zero on GC tracking #84051

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
346b9fe
Initial work
TIHan Mar 29, 2023
935e09a
Added genCodeForReuseVal
TIHan Mar 29, 2023
3e6c2f8
Only create a label if the current IG is not empty
TIHan Mar 29, 2023
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
1 change: 1 addition & 0 deletions src/coreclr/jit/codegen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1120,6 +1120,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
void genCodeForPhysReg(GenTreePhysReg* tree);
void genCodeForNullCheck(GenTreeIndir* tree);
void genCodeForCmpXchg(GenTreeCmpXchg* tree);
void genCodeForReuseVal(GenTree* treeNode);

void genAlignStackBeforeCall(GenTreePutArgStk* putArgStk);
void genAlignStackBeforeCall(GenTreeCall* call);
Expand Down
4 changes: 1 addition & 3 deletions src/coreclr/jit/codegenarmarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,7 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
// setting the GTF_REUSE_REG_VAL flag.
if (treeNode->IsReuseRegVal())
{
// For now, this is only used for constant nodes.
assert(treeNode->OperIs(GT_CNS_INT, GT_CNS_DBL, GT_CNS_VEC));
JITDUMP(" TreeNode is marked ReuseReg\n");
genCodeForReuseVal(treeNode);
return;
}

Expand Down
27 changes: 27 additions & 0 deletions src/coreclr/jit/codegencommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9442,3 +9442,30 @@ bool CodeGen::genCanOmitNormalizationForBswap16(GenTree* tree)

return (cast->gtCastType == TYP_USHORT) || (cast->gtCastType == TYP_SHORT);
}

//----------------------------------------------------------------------
// genCodeForReuseVal: Generate code for a node marked with re-using a register.
//
// Arguments:
// tree - The node marked with re-using a register
//
// Remarks:
// Generates nothing, except for when the node is a CNS_INT(0) where
// we will define a new label to propagate GC info. We want to do this
// because if the node is a CNS_INT(0) and is re-using a register,
// that register could have been used for a CNS_INT(ref null) that is GC
// tracked.
//
void CodeGen::genCodeForReuseVal(GenTree* treeNode)
{
assert(treeNode->IsReuseRegVal());

// For now, this is only used for constant nodes.
assert(treeNode->OperIs(GT_CNS_INT, GT_CNS_DBL, GT_CNS_VEC));
JITDUMP(" TreeNode is marked ReuseReg\n");

if (treeNode->IsIntegralConst(0) && GetEmitter()->emitCurIGnonEmpty())
{
genDefineTempLabel(genCreateTempLabel());
Copy link
Contributor Author

@TIHan TIHan Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be the only way to propagate GC info properly.

Copy link
Contributor Author

@TIHan TIHan Mar 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BruceForstall you are probably best to look at this.

The main issue is that we have a null node and a cns_int 0 node re-using the same register, but when that register is being used for cns_int 0, there are circumstances where the GC info hasn't propagated yet and that register is still seen as GC tracked even if it's for an integer. This happens if both the null and cns_int 0 node live within the same IG.

Originally I wanted to simply not GC track a reg that is initially null but that caused many seg faults. I tried that here: 346b9fe
If that worked, then we wouldn't have to worry about GC tracking when registers get re-used.

The only other option, I think, is to propagate the GC info by creating a new label, which creates a new IG; serves as a snapshot of the GC info.

You have any thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think the right fix is something like

gcInfo.gcMarkRegPtrVal(tree->getRegNum(), tree->TypeGet());

Copy link
Contributor Author

@TIHan TIHan Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AndyAyersMS In order for the state of gcInfo to be reflected in the actual emitting of instructions, it has to be propagated via instruction groups, which is what the creation of the label is doing. In this case, the state of gcInfo has already marked the register as a non-pointer, but that information wasn't propagated. gcMarkRegPtrVal doesn't propagate the info, it just sets the state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does propagate it, but only during codegen. I suspect the emitter GC tracking doesn't handle a case like this where the gcness of a register can change without writing to the register.

In which case, you might consider introducing a fake instruction that just reinterprets the register with its new GC type and acts like a write; that might convey intention of changing GC type more clearly than forcing a label, which seems a bit fragile and obscure.

Or it might gum up other things. Hard to say.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might consider introducing a fake instruction

Ah, I see Bruce had similar comments below.

I agree we can take this fix for now, but we should seriously consider something a bit more explicit. Ideally the code generator would not need to be aware of the peculiar aspects of emitter gc tracking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should introduce a fake instruction, this is what @BruceForstall and I talked about. Seems Bruce beat me to making an issue about it. :)

}
}
4 changes: 1 addition & 3 deletions src/coreclr/jit/codegenxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1704,9 +1704,7 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode)
// setting the GTF_REUSE_REG_VAL flag.
if (treeNode->IsReuseRegVal())
{
// For now, this is only used for constant nodes.
assert((treeNode->OperIsConst()));
JITDUMP(" TreeNode is marked ReuseReg\n");
genCodeForReuseVal(treeNode);
return;
}

Expand Down