Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
20 changes: 11 additions & 9 deletions src/coreclr/src/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -3496,19 +3496,21 @@ class Compiler
assert(varDsc->lvSize() == 16);
#endif // defined(TARGET_64BIT)

// We make local variable SIMD12 types 16 bytes instead of just 12. lvSize()
// already does this calculation. However, we also need to prevent mapping types if the var is a
// dependently promoted struct field, which must remain its exact size within its parent struct.
// However, we don't know this until late, so we may have already pretended the field is bigger
// before that.
if ((varDsc->lvSize() == 16) && !lvaIsFieldOfDependentlyPromotedStruct(varDsc))
// We make local variable SIMD12 types 16 bytes instead of just 12.
// lvSize() will return 16 bytes for SIMD12, even for fields.
// However, we can't do that mapping if the var is a dependently promoted struct field.
// Such a field must remain its exact size within its parent struct unless it is a single
// field *and* it is the only field in a struct of 16 bytes.
if (varDsc->lvSize() != 16)
{
return true;
return false;
}
else
if (lvaIsFieldOfDependentlyPromotedStruct(varDsc))
{
return false;
LclVarDsc* parentVarDsc = lvaGetDesc(varDsc->lvParentLcl);
return (parentVarDsc->lvFieldCnt == 1) && (parentVarDsc->lvSize() == 16);
}
return true;
}
#endif // defined(FEATURE_SIMD)

Expand Down
47 changes: 27 additions & 20 deletions src/coreclr/src/jit/morph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3665,31 +3665,21 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call)
#else // UNIX_AMD64_ABI
// On Unix, structs are always passed by value.
// We only need a copy if we have one of the following:
// - We have a lclVar that has been promoted and is passed in registers.
// - The sizes don't match for a non-lclVar argument.
// - We have a known struct type (e.g. SIMD) that requires multiple registers.
// TODO-Amd64-Unix-CQ: The first case could and should be handled without copies.
// TODO-Amd64-Unix-Throughput: We don't need to keep the structDesc in the argEntry if it's not
// actually passed in registers.
if (argEntry->isPassedInRegisters())
{
assert(argEntry->structDesc.passedInRegisters);
if (lclVar != nullptr)
{
if (lvaGetPromotionType(lclVar->AsLclVarCommon()->GetLclNum()) ==
PROMOTION_TYPE_INDEPENDENT)
{
copyBlkClass = objClass;
}
}
else if (argObj->OperIs(GT_OBJ))
if (argObj->OperIs(GT_OBJ))
{
if (passingSize != structSize)
{
copyBlkClass = objClass;
}
}
else
else if (lclVar == nullptr)
{
// This should only be the case of a value directly producing a known struct type.
assert(argObj->TypeGet() != TYP_STRUCT);
Expand Down Expand Up @@ -9720,7 +9710,9 @@ GenTree* Compiler::fgMorphInitBlock(GenTree* tree)
}
#endif // LOCAL_ASSERTION_PROP

if (destLclVar->lvPromoted)
// If we have already determined that a promoted TYP_STRUCT lclVar will not be enregistered,
// we are better off doing a block init.
if (destLclVar->lvPromoted && (!destLclVar->lvDoNotEnregister || !destLclNode->TypeIs(TYP_STRUCT)))
{
GenTree* newTree = fgMorphPromoteLocalInitBlock(destLclNode->AsLclVar(), initVal, blockSize);

Expand Down Expand Up @@ -10604,15 +10596,30 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
// Are both dest and src promoted structs?
if (destDoFldAsg && srcDoFldAsg)
{
// Both structs should be of the same type, or each have a single field of the same type.
// Both structs should be of the same type, or have the same number of fields of the same type.
// If not we will use a copy block.
if (lvaTable[destLclNum].lvVerTypeInfo.GetClassHandle() !=
lvaTable[srcLclNum].lvVerTypeInfo.GetClassHandle())
bool misMatchedTypes = false;
if (destLclVar->lvVerTypeInfo.GetClassHandle() != srcLclVar->lvVerTypeInfo.GetClassHandle())
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you give me an example when do we generate ASG between structs with different struct handles?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are at least two ways this can happen:

  • Generics "mismatch". The case that first called my attention to this was a mismatch between System.Span<Canon> and System.Span<String> in an assignment due to an inline.
  • You can have a struct that's nested within another struct in which it's the only field.

There were about 1500 instances of this on linux/x64 I believe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the explanation.

{
unsigned destFieldNum = lvaTable[destLclNum].lvFieldLclStart;
unsigned srcFieldNum = lvaTable[srcLclNum].lvFieldLclStart;
if ((lvaTable[destLclNum].lvFieldCnt != 1) || (lvaTable[srcLclNum].lvFieldCnt != 1) ||
(lvaTable[destFieldNum].lvType != lvaTable[srcFieldNum].lvType))
if (destLclVar->lvFieldCnt != srcLclVar->lvFieldCnt)
{
misMatchedTypes = true;
}
else
{
for (int i = 0; i < destLclVar->lvFieldCnt; i++)
{
LclVarDsc* destFieldVarDsc = lvaGetDesc(destLclVar->lvFieldLclStart + i);
LclVarDsc* srcFieldVarDsc = lvaGetDesc(srcLclVar->lvFieldLclStart + i);
if ((destFieldVarDsc->lvType != srcFieldVarDsc->lvType) ||
(destFieldVarDsc->lvFldOffset != srcFieldVarDsc->lvFldOffset))
{
misMatchedTypes = true;
break;
}
}
}
if (misMatchedTypes)
{
requiresCopyBlock = true; // Mismatched types, leave as a CopyBlock
JITDUMP(" with mismatched types");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we leave one of them (dst or src) as an independently promoted struct? Set destDoFldAsg or srcDoFldAsg to false and do copies from memory to registers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an interesting question. I don't want to hold up this PR, but I'll file an issue for that.

Expand Down