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

Use ClassLayout::AreCompatible in fgMorphCanUseLclFldForCopy #69642

Closed
wants to merge 1 commit into from
Closed

Use ClassLayout::AreCompatible in fgMorphCanUseLclFldForCopy #69642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 1 addition & 4 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,10 +868,7 @@ void Compiler::impPopCallArgs(CORINFO_SIG_INFO* sig, GenTreeCall* call)
// the parameter type but exact types in the signature type.
//
// TODO-ARGS: Remove this quirk; we should be able to use the
// signature type that is different in the rare case above. It will
// cause positive diffs, but that is probably an indication that we
// have downstream phases that should be using
// `ClassLayout::AreCompatible` instead.
// signature type that is different in the rare case above.
//
classHnd = ti.GetClassHandleForValueClass();

Expand Down
16 changes: 2 additions & 14 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9652,11 +9652,7 @@ GenTree* Compiler::fgMorphBlockOperand(GenTree* tree, var_types asgType, unsigne
// lclNum2 - the local variable on the other side of ASG, can be BAD_VAR_NUM.
//
// Return Value:
// True if the second local is valid and has the same struct handle as the first,
// false otherwise.
//
// Notes:
// TODO-PhysicalVN: with the physical VN scheme, this method is no longer needed.
// True if the second local is valid and has a layout compatible with the first local.
Comment on lines -9659 to +9655
Copy link
Contributor

@SingleAccretion SingleAccretion May 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The TODO should stay or the whole method deleted (what #68986 does).

There is no reason we cannot use LCL_FLD<lcl> in a construct such as the following:

lcl.StructField = promotedLocal;

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I have not noticed that #68986 removes this, I will just leave it for that PR then.

There is no reason we cannot use LCL_FLD in a construct such as the following:

I see, makes sense. My main worry here was about dropping GC ness during the copy, but I guess the alternatives do not do anything different in that regard.

//
bool Compiler::fgMorphCanUseLclFldForCopy(unsigned lclNum1, unsigned lclNum2)
{
Expand All @@ -9672,15 +9668,7 @@ bool Compiler::fgMorphCanUseLclFldForCopy(unsigned lclNum1, unsigned lclNum2)
{
return false;
}
CORINFO_CLASS_HANDLE struct1 = varDsc1->GetStructHnd();
CORINFO_CLASS_HANDLE struct2 = varDsc2->GetStructHnd();
assert(struct1 != NO_CLASS_HANDLE);
assert(struct2 != NO_CLASS_HANDLE);
if (struct1 != struct2)
{
return false;
}
return true;
return ClassLayout::AreCompatible(varDsc1->GetLayout(), varDsc2->GetLayout());
}

// insert conversions and normalize to make tree amenable to register
Expand Down