Do not promote struct locals with holes by kunalspathak · Pull Request #62645 · dotnet/runtime

kunalspathak · 2021-12-10T18:36:29Z

In #43870 , when we added more support for multireg args, we added a check for struct's field count. We didn't check if the total size of struct matched the combined size of both the fields. That led us to generate code that partially copy the struct fields on stack when passing as arguments. Add an explicit check to verify the combined size matches.

Fixes: #62597

ghost · 2021-12-10T18:36:35Z

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

In #43870 , when we added more support for multireg args, we added a check for struct's field count. We didn't check if the total size of struct matched the combined size of both the fields. That led us to generate code that partially copy the struct fields on stack when passing as arguments. Add an explicit check to verify the combined size matches.

Fixes: #62597

Author:	kunalspathak
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

kunalspathak · 2021-12-10T18:36:39Z

/azp list

azure-pipelines · 2021-12-10T18:36:44Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr jitstress runtime-coreclr jitstressregs runtime-coreclr jitstress2-jitstressregs runtime-coreclr gcstress0x3-gcstress0xc runtime-coreclr gcstress-extra runtime-coreclr r2r-extra runtime-coreclr jitstress-isas-x86 runtime-coreclr jitstress-isas-arm runtime-coreclr jitstressregs-x86 runtime-coreclr libraries-jitstressregs runtime-coreclr libraries-jitstress2-jitstressregs runtime-coreclr r2r runtime-coreclr runincontext runtime-coreclr crossgen2 runtime-libraries-coreclr outerloop runtime-libraries-coreclr outerloop-windows runtime-libraries-coreclr outerloop-linux runtime-libraries-coreclr outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-dev-innerloop runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono runtime-coreclr crossgen2-composite runtime-jit-experimental runtime-coreclr libraries-jitstress dotnet-linker-tests runtime-coreclr ilasm runtime-coreclr crossgen2-composite gcstress runtime-libraries-mono outerloop runtime-staging runtime-coreclr pgo runtime-coreclr libraries-pgo coreclr-gc-regions Antigen runtime-community Fuzzlyn runtime-coreclr superpmi-replay wasm-debugger runtime-coreclr superpmi-asmdiffs runtime-coreclr superpmi-asmdiff-checked-release runtime-manual runtime-extra-platforms

kunalspathak · 2021-12-10T18:38:02Z

/azp run runtime-coreclr jitstress
/azp run runtime-coreclr jitstressregs
/azp run runtime-coreclr libraries-jitstress
/azp run Antigen
/azp run Fuzzlyn

azure-pipelines · 2021-12-10T18:38:08Z

No pipelines are associated with this pull request.

kunalspathak · 2021-12-10T18:38:22Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2021-12-10T18:38:38Z

Azure Pipelines successfully started running 1 pipeline(s).

kunalspathak · 2021-12-10T18:38:45Z

/azp run runtime-coreclr jitstressregs

azure-pipelines · 2021-12-10T18:39:01Z

Azure Pipelines successfully started running 1 pipeline(s).

kunalspathak · 2021-12-10T18:39:09Z

/azp run runtime-coreclr libraries-jitstress

azure-pipelines · 2021-12-10T18:39:26Z

Azure Pipelines successfully started running 1 pipeline(s).

kunalspathak · 2021-12-10T18:39:35Z

/azp run Antigen

kunalspathak · 2021-12-10T18:39:39Z

/azp run Fuzzlyn

azure-pipelines · 2021-12-10T18:39:52Z

Azure Pipelines successfully started running 1 pipeline(s).

kunalspathak · 2021-12-10T18:39:53Z

@dotnet/jit-contrib

azure-pipelines · 2021-12-10T18:39:55Z

Azure Pipelines successfully started running 1 pipeline(s).

BruceForstall · 2021-12-10T19:19:18Z

+ if (structPromotionInfo.fieldCnt == 2)
+ {
+ unsigned structSize = varDsc->GetLayout()->GetSize();
+ if (structPromotionInfo.fields[0].fldSize * 2 != structSize)


Wouldn't this be better and more general?
Suggested change
if (structPromotionInfo.fields[0].fldSize * 2 != structSize)
if (structPromotionInfo.containsHoles)
(e.g., why *2 instead of fields[0].fldSize + fields[1].fldSize?)
Seems like the next condition maybe should also check containsHoles in case there's an explicit layout struct of a single SIMD field where the explicit layout is larger than the SIMD field

Ah, I actually had fields[0].fldSize + fields[1].fldSize but somehow I didn't push that change. Let me do it.

Looking at the code around, containsHoles does exactly that, so I should just replace this check with containHoles. Perhaps I should also add a check for customLayout the way it is done here:
runtime/src/coreclr/jit/lclvars.cpp
Lines 2018 to 2021 in 7dcb289
elseif (varDsc->lvIsMultiRegRet && structPromotionInfo.containsHoles && structPromotionInfo.customLayout)
{
JITDUMP("Not promoting multi-reg returned struct local V%02u with holes.\n", lclNum);
shouldPromote = false;

Yes, probably should add && structPromotionInfo.customLayout as well. I think the logic here is: a default/automatic struct layout might end up with holes due to alignment padding, but users can't depend on values in that padding. However, if the user explicitly set a custom/explicit layout, then we can't depend on the padding/holes being uninteresting.

AndyAyersMS · 2021-12-10T19:30:29Z

Is there some easy way to repro this without generating IL at runtime?

Dynamic methods bypass tiering (and thus OSR/PGO) and so we won't get as much coverage as we would from a source example.

kunalspathak · 2021-12-10T19:46:54Z

Is there some easy way to repro this without generating IL at runtime?

I will try to find out.

BruceForstall

LGTM

Any asm diffs?

Presumably this is a back-port candidate

kunalspathak · 2021-12-10T22:57:49Z

Any asm diffs?

I will post the result once the asmdiff pipeline completes.

Presumably this is a back-port candidate

That's right. I will start the process once I see the asmdiffs.

kunalspathak · 2021-12-10T22:58:10Z

/azp run runtime-coreclr jitstress

kunalspathak · 2021-12-10T22:58:16Z

/azp run runtime-coreclr jitstressregs

kunalspathak · 2021-12-10T22:58:23Z

/azp run runtime-coreclr libraries-jitstress

azure-pipelines · 2021-12-10T22:58:25Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2021-12-10T22:58:32Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2021-12-10T22:58:38Z

Azure Pipelines successfully started running 1 pipeline(s).

kunalspathak · 2021-12-11T00:18:05Z

asmdiffs results: https://dev.azure.com/dnceng/public/_build/results?buildId=1507589&view=ms.vss-build-web.run-extensions-tab

The worst difference are around 228 bytes in windows/linux arm64. I also inspected diffs from other platforms, and almost all of them are coming because we stop promoting the DateTimeOffset struct. So the code changes are related to _offsetMinutes and _offsetMinutes as they are passed in memory now.

runtime/src/libraries/System.Private.CoreLib/src/System/DateTimeOffset.cs

Lines 32 to 65 in 06bb200

[StructLayout(LayoutKind.Auto)]
[Serializable]
[System.Runtime.CompilerServices.TypeForwardedFrom("mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
publicreadonlystructDateTimeOffset:IComparable,ISpanFormattable,IComparable<DateTimeOffset>,IEquatable<DateTimeOffset>,ISerializable,IDeserializationCallback
 #if FEATURE_GENERIC_MATH 
 #pragma warning disable SA1001,CA2252// SA1001: Comma positioning; CA2252: Preview Features
,IAdditionOperators<DateTimeOffset,TimeSpan,DateTimeOffset>,
IAdditiveIdentity<DateTimeOffset,TimeSpan>,
IComparisonOperators<DateTimeOffset,DateTimeOffset>,
IMinMaxValue<DateTimeOffset>,
ISpanParseable<DateTimeOffset>,
ISubtractionOperators<DateTimeOffset,TimeSpan,DateTimeOffset>,
ISubtractionOperators<DateTimeOffset,DateTimeOffset,TimeSpan>
 #pragma warning restore SA1001,CA2252
 #endif // FEATURE_GENERIC_MATH
{
// Constants
internalconstlongMaxOffset=TimeSpan.TicksPerHour*14;
internalconstlongMinOffset=-MaxOffset;
privateconstlongUnixEpochSeconds=DateTime.UnixEpochTicks/TimeSpan.TicksPerSecond;// 62,135,596,800
privateconstlongUnixEpochMilliseconds=DateTime.UnixEpochTicks/TimeSpan.TicksPerMillisecond;// 62,135,596,800,000
internalconstlongUnixMinSeconds=DateTime.MinTicks/TimeSpan.TicksPerSecond-UnixEpochSeconds;
internalconstlongUnixMaxSeconds=DateTime.MaxTicks/TimeSpan.TicksPerSecond-UnixEpochSeconds;
// Static Fields
publicstaticreadonlyDateTimeOffsetMinValue=newDateTimeOffset(DateTime.MinTicks,TimeSpan.Zero);
publicstaticreadonlyDateTimeOffsetMaxValue=newDateTimeOffset(DateTime.MaxTicks,TimeSpan.Zero);
publicstaticreadonlyDateTimeOffsetUnixEpoch=newDateTimeOffset(DateTime.UnixEpochTicks,TimeSpan.Zero);
// Instance Fields
privatereadonlyDateTime_dateTime;
privatereadonlyshort_offsetMinutes;

ltrzesniewski · 2021-12-11T01:20:52Z

I took a quick glance at the code to better understand what's going on, and I noticed something: a comment in ShouldPromoteStructVar says: We can promote; should we promote?
Shouldn't that case (structPromotionInfo.containsHoles && structPromotionInfo.customLayout) always mean we can't promote? So shouldn't it be checked in CanPromoteStructVar or even in CanPromoteStructType instead?

kunalspathak · 2021-12-13T18:33:43Z

Failures are related to #60648, #58704 and #52361.

kunalspathak · 2021-12-13T18:35:42Z

I took a quick glance at the code to better understand what's going on, and I noticed something: a comment in ShouldPromoteStructVar says: We can promote; should we promote? Shouldn't that case (structPromotionInfo.containsHoles && structPromotionInfo.customLayout) always mean we can't promote? So shouldn't it be checked in CanPromoteStructVar or even in CanPromoteStructType instead?

Your observations are correct @ltrzesniewski. However, currently, the code that checks for "can" and "should" are intermixed and probably we need to separate out some day. I just wanted to have the fix close to the place where we do similar check for "returned struct":

runtime/src/coreclr/jit/lclvars.cpp

Lines 2018 to 2021 in f18f658

elseif (varDsc->lvIsMultiRegRet && structPromotionInfo.containsHoles && structPromotionInfo.customLayout) 
 { 
JITDUMP("Not promoting multi-reg returned struct local V%02u with holes.\n", lclNum); 
 shouldPromote = false; 

kunalspathak · 2021-12-13T18:37:29Z

/backport to release/6.0

github-actions · 2021-12-13T18:37:43Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1574362543

Make sure the combined field size matches the struct size
ef56bf3

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 10, 2021

BruceForstall suggested changes Dec 10, 2021
View reviewed changes

ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Dec 10, 2021

Fix the condition
674a5aa

Update test and include containHoles condition
f26d8ff

kunalspathak changed the title ~~Make sure the combined field size matches the struct size~~Do not promote struct locals with holesDec 10, 2021

BruceForstall approved these changes Dec 10, 2021
View reviewed changes

kunalspathak merged commit bffa7cf into dotnet:mainDec 13, 2021

github-actionsBot mentioned this pull request Dec 13, 2021
[release/6.0] Do not promote struct locals with holes #62738
Merged

ghost locked as resolved and limited conversation to collaborators Jan 12, 2022

	if (structPromotionInfo.fields[0].fldSize * 2 != structSize)
	if (structPromotionInfo.containsHoles)

	elseif (varDsc->lvIsMultiRegRet && structPromotionInfo.containsHoles && structPromotionInfo.customLayout)
	{
	JITDUMP("Not promoting multi-reg returned struct local V%02u with holes.\n", lclNum);
	shouldPromote = false;

Uh oh!

Conversation

kunalspathak commented Dec 10, 2021

Uh oh!

ghost commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

azure-pipelinesBot commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

azure-pipelinesBot commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

azure-pipelinesBot commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

azure-pipelinesBot commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

azure-pipelinesBot commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

azure-pipelinesBot commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

azure-pipelinesBot commented Dec 10, 2021

Uh oh!

BruceForstallDec 10, 2021

Choose a reason for hiding this comment

Uh oh!

kunalspathakDec 10, 2021

Choose a reason for hiding this comment

Uh oh!

kunalspathakDec 10, 2021

Choose a reason for hiding this comment

Uh oh!

BruceForstallDec 10, 2021

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 10, 2021

Uh oh!

azure-pipelinesBot commented Dec 10, 2021

Uh oh!

azure-pipelinesBot commented Dec 10, 2021

Uh oh!

azure-pipelinesBot commented Dec 10, 2021

Uh oh!

kunalspathak commented Dec 11, 2021

Uh oh!

ltrzesniewski commented Dec 11, 2021

Uh oh!

kunalspathak commented Dec 13, 2021

Uh oh!

kunalspathak commented Dec 13, 2021

Uh oh!

kunalspathak commented Dec 13, 2021

Uh oh!

github-actionsBot commented Dec 13, 2021