Crossgen2: Support HVA for ARM64 by AntonLapounov · Pull Request #35576 · dotnet/runtime

AntonLapounov · 2020-04-28T17:15:58Z

Support ARM64 HVAs in Crossgen2.

Change FieldLayoutAlgorithm.ComputeValueTypeShapeCharacteristics method to compute the homogeneous aggregate element type and cache it in the existing field. That allows to remove all ComputeHomogeneousFloatAggregateElementType methods.
Change MetadataFieldLayoutAlgorithm.ComputeHomogeneousAggregateCharacteristic to compute HVAs in addition to HFAs.
Change CorInfoImpl.getHFAType JIT callback to handle HVAs. Note that returning ELEMENT_TYPE_VALUETYPE indicates the TYP_SIMD16 type (see Compiler::GetHfaType).
Change TypeFixupSignature.EncodeTypeLayout to handle HVAs.
Support HVAs in the ArgIterator class.
Fix TransitionBlock.OffsetFromGCRefMapPos for ARM64. R2RDump used to dump incorrect offsets.
Use TransitionBlock.OffsetFromGCRefMapPos in GCRefMapBuilder.GetCallRefMap to simplify logic.
Remove ARM64 .NET Native-specific code from TransitionBlock.cs and ArgIterator.cs files.

Minor:

Remove a redundant GetVectorSize check in MethodTable::GetHFAType.
Improve an assertion in Compiler::raUpdateRegStateForArg.
Fix comments in JIT code.

AntonLapounov · 2020-04-28T17:21:08Z

- vectorSize = pMT->GetVectorSize();
- if (vectorSize != 0)
- {
- return (vectorSize == 8) ? ELEMENT_TYPE_R8 : ELEMENT_TYPE_VALUETYPE;
- }


The next loop iteration performs this check anyway, so this one is redundant.

AntonLapounov · 2020-04-28T17:28:58Z

 }
 else
 {
+ assert(!regState->rsIsFloat);


We could miss this check due to the break below, then fail later in an unexpected way.

cc: @dotnet/jit-contrib

MichalStrehovsky · 2020-04-29T09:16:38Z

We have xUnit test coverage for these aspects in the CoreRT repo: https://github.com/dotnet/corert/blob/923eeee2fb70497fe07b74d56bc51bee029c108b/src/ILCompiler.TypeSystem/tests/ValueTypeShapeCharacteristicsTests.cs

Issue #200 tracks porting these xUnit tests to the runtime repo, but for now they only compile and run in the experimental CoreRT repo (they will run if you just build.cmd from the root of the repo). I don't know if porting them is on anyone's radar soon (it would be about figuring out how to run xUnit as part of the coreclr managed tools build). Cc @dotnet/crossgen-contrib on that.

We haven't made significant type system changes in the runtime repo yet, so there's no process for this.

I sync the type system and compiler back to the CoreRT repo on weekends when I have time. Truth to be told, when I port this over and tests stop working, I'm just going to delete the tests because I don't have that much time.

If it's not too much hassle, could you do this change in the CoreRT repo too, and adjust/add test coverage as needed? Type system bugs tend to be subtle and that's why we try to unit test as much of it as possible.

You should be able to just xcopy src\coreclr\src\tools\Common\TypeSystem to src\Common\src\TypeSystem and src\coreclr\src\tools\Common\JitInterface to src\JitInterface\src. There's about two weeks worth of diffs because I didn't sync in 2 weeks, but it should be manageable to just undo irrelevant changes.

davidwrighton

I'd like to see some effect on GCStress numbers. @janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

davidwrighton · 2020-04-29T21:10:54Z

 {
- int ofs;
-
- if (_target.Architecture == TargetArchitecture.X86)


I realize that we don't yet support Crossgen2 for x86, but is removing this special case correct? Should it instead be moved to the OffsetFromGCRefMapPos function?

This special case is already in OffsetFromGCRefMapPos overload for x86:
runtime/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TransitionBlock.cs
Lines 375 to 385 in 6ce7610
publicoverrideintOffsetFromGCRefMapPos(intpos)
{
if(pos<NumArgumentRegisters)
{
returnOffsetOfArgumentRegisters+SizeOfArgumentRegisters-(pos+1)*PointerSize;
}
else
{
returnOffsetOfArgs+(pos-NumArgumentRegisters)*PointerSize;
}
}

janvorli · 2020-04-29T21:47:19Z

@janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

Running with GC stress enabled is as easy as passing --gcstress argument to r2rtest tool (e.g. --gcstress 3). Or, if you are using runtest.cmd, then passing in gcstresslevel argument (and runcrossgen2tests to use crossgen2).

janvorli

LGTM, thank you!

MichalStrehovsky · 2020-05-01T17:25:49Z

I sync the type system and compiler back to the CoreRT repo on weekends when I have time

Since we had a public holiday in Slovakia, managed to sync the compiler a bit earlier. dotnet/corert#8122.

AntonLapounov · 2020-05-01T19:48:52Z

@davidwrighton Below are results of --gcstress 3 for pri-0 tests after the changes. Before the changes the framework could not be compiled with crossgen2.

Compilation	Crossgen	CPAOT
PASS	3987	3998
FAIL	14	3
Total	4001	4001

Execution	Crossgen	CPAOT
PASS	2765	2754
EXIT_CODE	8	18
CRASHED	0	0
TIMED_OUT	6	6
BUILD_FAILED	2	3
Total	2781	2781

AntonLapounov · 2020-05-01T20:17:47Z

@echesakovMSFT This PR includes minor changes under jit and vm. It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type. Otherwise, JIT fails later in the prolog codegen phase or the register allocation phase.

AntonLapounov · 2020-05-01T21:26:15Z

@CarolEidt Implementation of HFA/HVA property calculation in this PR is free of issues documented in #35144. We need them to be fixed so that Crossgen2 compiler and VM agree on the calling convention. One possible way of fixing that is to introduce a new enumeration for fundamental data types of homogeneous aggregates, like I did in this PR: https://github.com/dotnet/runtime/pull/35576/files#diff-e553d696b7062596f5653d5bba53d278, and use it as the return type of the getHFAType callback. I also suggested adding an assertion for its returned value: #35576 (comment).

CarolEidt · 2020-05-02T00:12:47Z

It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type.

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@AntonLapounov - I was previously thinking that fixing #35144 would require a change to the JIT/EE interface, but I believe it's the case that from a JIT perspective it doesn't really need to distinguish between an HFA of double and an HVA of SIMD8. So I think the fix just needs to be made on the vm side of the JIT/EE interface, as you've done here.

CarolEidt

The JIT changes LGTM

AntonLapounov · 2020-05-02T00:51:25Z

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@CarolEidt I would prefer someone with more JIT knowledge to do that. You are right that technically we do not have to change the getHFA API; however, I find using ELEMENT_TYPE_VALUETYPE to actually mean HfaElemKind.HFA_ELEM_SIMD16 (apparently JIT already has this enumeration) less than perfect.

janvorli · 2020-05-04T10:53:36Z

@AntonLapounov as for the timed out tests, can you please try to bump the execution timeout using the --execution-timeout-minutes r2rtest option to see if the timeouts are just taking more time or they represent hangs? You can try to bump it to e.g. 60 minutes to give it enough space.

AntonLapounov · 2020-04-28T17:15:58Z

Support ARM64 HVAs in Crossgen2.

Change FieldLayoutAlgorithm.ComputeValueTypeShapeCharacteristics method to compute the homogeneous aggregate element type and cache it in the existing field. That allows to remove all ComputeHomogeneousFloatAggregateElementType methods.
Change MetadataFieldLayoutAlgorithm.ComputeHomogeneousAggregateCharacteristic to compute HVAs in addition to HFAs.
Change CorInfoImpl.getHFAType JIT callback to handle HVAs. Note that returning ELEMENT_TYPE_VALUETYPE indicates the TYP_SIMD16 type (see Compiler::GetHfaType).
Change TypeFixupSignature.EncodeTypeLayout to handle HVAs.
Support HVAs in the ArgIterator class.
Fix TransitionBlock.OffsetFromGCRefMapPos for ARM64. R2RDump used to dump incorrect offsets.
Use TransitionBlock.OffsetFromGCRefMapPos in GCRefMapBuilder.GetCallRefMap to simplify logic.
Remove ARM64 .NET Native-specific code from TransitionBlock.cs and ArgIterator.cs files.

Minor:

Remove a redundant GetVectorSize check in MethodTable::GetHFAType.
Improve an assertion in Compiler::raUpdateRegStateForArg.
Fix comments in JIT code.

AntonLapounov · 2020-04-28T17:21:08Z

- vectorSize = pMT->GetVectorSize();
- if (vectorSize != 0)
- {
- return (vectorSize == 8) ? ELEMENT_TYPE_R8 : ELEMENT_TYPE_VALUETYPE;
- }


The next loop iteration performs this check anyway, so this one is redundant.

AntonLapounov · 2020-04-28T17:28:58Z

 }
 else
 {
+ assert(!regState->rsIsFloat);


We could miss this check due to the break below, then fail later in an unexpected way.

cc: @dotnet/jit-contrib

MichalStrehovsky · 2020-04-29T09:16:38Z

We have xUnit test coverage for these aspects in the CoreRT repo: https://github.com/dotnet/corert/blob/923eeee2fb70497fe07b74d56bc51bee029c108b/src/ILCompiler.TypeSystem/tests/ValueTypeShapeCharacteristicsTests.cs

Issue #200 tracks porting these xUnit tests to the runtime repo, but for now they only compile and run in the experimental CoreRT repo (they will run if you just build.cmd from the root of the repo). I don't know if porting them is on anyone's radar soon (it would be about figuring out how to run xUnit as part of the coreclr managed tools build). Cc @dotnet/crossgen-contrib on that.

We haven't made significant type system changes in the runtime repo yet, so there's no process for this.

I sync the type system and compiler back to the CoreRT repo on weekends when I have time. Truth to be told, when I port this over and tests stop working, I'm just going to delete the tests because I don't have that much time.

If it's not too much hassle, could you do this change in the CoreRT repo too, and adjust/add test coverage as needed? Type system bugs tend to be subtle and that's why we try to unit test as much of it as possible.

You should be able to just xcopy src\coreclr\src\tools\Common\TypeSystem to src\Common\src\TypeSystem and src\coreclr\src\tools\Common\JitInterface to src\JitInterface\src. There's about two weeks worth of diffs because I didn't sync in 2 weeks, but it should be manageable to just undo irrelevant changes.

davidwrighton

I'd like to see some effect on GCStress numbers. @janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

davidwrighton · 2020-04-29T21:10:54Z

 {
- int ofs;
-
- if (_target.Architecture == TargetArchitecture.X86)


I realize that we don't yet support Crossgen2 for x86, but is removing this special case correct? Should it instead be moved to the OffsetFromGCRefMapPos function?

This special case is already in OffsetFromGCRefMapPos overload for x86:
runtime/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TransitionBlock.cs
Lines 375 to 385 in 6ce7610
publicoverrideintOffsetFromGCRefMapPos(intpos)
{
if(pos<NumArgumentRegisters)
{
returnOffsetOfArgumentRegisters+SizeOfArgumentRegisters-(pos+1)*PointerSize;
}
else
{
returnOffsetOfArgs+(pos-NumArgumentRegisters)*PointerSize;
}
}

janvorli · 2020-04-29T21:47:19Z

@janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

Running with GC stress enabled is as easy as passing --gcstress argument to r2rtest tool (e.g. --gcstress 3). Or, if you are using runtest.cmd, then passing in gcstresslevel argument (and runcrossgen2tests to use crossgen2).

janvorli

LGTM, thank you!

MichalStrehovsky · 2020-05-01T17:25:49Z

I sync the type system and compiler back to the CoreRT repo on weekends when I have time

Since we had a public holiday in Slovakia, managed to sync the compiler a bit earlier. dotnet/corert#8122.

AntonLapounov · 2020-05-01T19:48:52Z

@davidwrighton Below are results of --gcstress 3 for pri-0 tests after the changes. Before the changes the framework could not be compiled with crossgen2.

Compilation	Crossgen	CPAOT
PASS	3987	3998
FAIL	14	3
Total	4001	4001

Execution	Crossgen	CPAOT
PASS	2765	2754
EXIT_CODE	8	18
CRASHED	0	0
TIMED_OUT	6	6
BUILD_FAILED	2	3
Total	2781	2781

AntonLapounov · 2020-05-01T20:17:47Z

@echesakovMSFT This PR includes minor changes under jit and vm. It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type. Otherwise, JIT fails later in the prolog codegen phase or the register allocation phase.

AntonLapounov · 2020-05-01T21:26:15Z

@CarolEidt Implementation of HFA/HVA property calculation in this PR is free of issues documented in #35144. We need them to be fixed so that Crossgen2 compiler and VM agree on the calling convention. One possible way of fixing that is to introduce a new enumeration for fundamental data types of homogeneous aggregates, like I did in this PR: https://github.com/dotnet/runtime/pull/35576/files#diff-e553d696b7062596f5653d5bba53d278, and use it as the return type of the getHFAType callback. I also suggested adding an assertion for its returned value: #35576 (comment).

CarolEidt · 2020-05-02T00:12:47Z

It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type.

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@AntonLapounov - I was previously thinking that fixing #35144 would require a change to the JIT/EE interface, but I believe it's the case that from a JIT perspective it doesn't really need to distinguish between an HFA of double and an HVA of SIMD8. So I think the fix just needs to be made on the vm side of the JIT/EE interface, as you've done here.

CarolEidt

The JIT changes LGTM

AntonLapounov · 2020-05-02T00:51:25Z

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@CarolEidt I would prefer someone with more JIT knowledge to do that. You are right that technically we do not have to change the getHFA API; however, I find using ELEMENT_TYPE_VALUETYPE to actually mean HfaElemKind.HFA_ELEM_SIMD16 (apparently JIT already has this enumeration) less than perfect.

janvorli · 2020-05-04T10:53:36Z

@AntonLapounov as for the timed out tests, can you please try to bump the execution timeout using the --execution-timeout-minutes r2rtest option to see if the timeouts are just taking more time or they represent hangs? You can try to bump it to e.g. 60 minutes to give it enough space.

AntonLapounov · 2020-04-28T17:15:58Z

Support ARM64 HVAs in Crossgen2.

Change FieldLayoutAlgorithm.ComputeValueTypeShapeCharacteristics method to compute the homogeneous aggregate element type and cache it in the existing field. That allows to remove all ComputeHomogeneousFloatAggregateElementType methods.
Change MetadataFieldLayoutAlgorithm.ComputeHomogeneousAggregateCharacteristic to compute HVAs in addition to HFAs.
Change CorInfoImpl.getHFAType JIT callback to handle HVAs. Note that returning ELEMENT_TYPE_VALUETYPE indicates the TYP_SIMD16 type (see Compiler::GetHfaType).
Change TypeFixupSignature.EncodeTypeLayout to handle HVAs.
Support HVAs in the ArgIterator class.
Fix TransitionBlock.OffsetFromGCRefMapPos for ARM64. R2RDump used to dump incorrect offsets.
Use TransitionBlock.OffsetFromGCRefMapPos in GCRefMapBuilder.GetCallRefMap to simplify logic.
Remove ARM64 .NET Native-specific code from TransitionBlock.cs and ArgIterator.cs files.

Minor:

Remove a redundant GetVectorSize check in MethodTable::GetHFAType.
Improve an assertion in Compiler::raUpdateRegStateForArg.
Fix comments in JIT code.

AntonLapounov · 2020-04-28T17:21:08Z

- vectorSize = pMT->GetVectorSize();
- if (vectorSize != 0)
- {
- return (vectorSize == 8) ? ELEMENT_TYPE_R8 : ELEMENT_TYPE_VALUETYPE;
- }


The next loop iteration performs this check anyway, so this one is redundant.

AntonLapounov · 2020-04-28T17:28:58Z

 }
 else
 {
+ assert(!regState->rsIsFloat);


We could miss this check due to the break below, then fail later in an unexpected way.

cc: @dotnet/jit-contrib

MichalStrehovsky · 2020-04-29T09:16:38Z

We have xUnit test coverage for these aspects in the CoreRT repo: https://github.com/dotnet/corert/blob/923eeee2fb70497fe07b74d56bc51bee029c108b/src/ILCompiler.TypeSystem/tests/ValueTypeShapeCharacteristicsTests.cs

Issue #200 tracks porting these xUnit tests to the runtime repo, but for now they only compile and run in the experimental CoreRT repo (they will run if you just build.cmd from the root of the repo). I don't know if porting them is on anyone's radar soon (it would be about figuring out how to run xUnit as part of the coreclr managed tools build). Cc @dotnet/crossgen-contrib on that.

We haven't made significant type system changes in the runtime repo yet, so there's no process for this.

I sync the type system and compiler back to the CoreRT repo on weekends when I have time. Truth to be told, when I port this over and tests stop working, I'm just going to delete the tests because I don't have that much time.

If it's not too much hassle, could you do this change in the CoreRT repo too, and adjust/add test coverage as needed? Type system bugs tend to be subtle and that's why we try to unit test as much of it as possible.

You should be able to just xcopy src\coreclr\src\tools\Common\TypeSystem to src\Common\src\TypeSystem and src\coreclr\src\tools\Common\JitInterface to src\JitInterface\src. There's about two weeks worth of diffs because I didn't sync in 2 weeks, but it should be manageable to just undo irrelevant changes.

davidwrighton

I'd like to see some effect on GCStress numbers. @janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

davidwrighton · 2020-04-29T21:10:54Z

 {
- int ofs;
-
- if (_target.Architecture == TargetArchitecture.X86)


I realize that we don't yet support Crossgen2 for x86, but is removing this special case correct? Should it instead be moved to the OffsetFromGCRefMapPos function?

This special case is already in OffsetFromGCRefMapPos overload for x86:
runtime/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TransitionBlock.cs
Lines 375 to 385 in 6ce7610
publicoverrideintOffsetFromGCRefMapPos(intpos)
{
if(pos<NumArgumentRegisters)
{
returnOffsetOfArgumentRegisters+SizeOfArgumentRegisters-(pos+1)*PointerSize;
}
else
{
returnOffsetOfArgs+(pos-NumArgumentRegisters)*PointerSize;
}
}

janvorli · 2020-04-29T21:47:19Z

@janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

Running with GC stress enabled is as easy as passing --gcstress argument to r2rtest tool (e.g. --gcstress 3). Or, if you are using runtest.cmd, then passing in gcstresslevel argument (and runcrossgen2tests to use crossgen2).

janvorli

LGTM, thank you!

MichalStrehovsky · 2020-05-01T17:25:49Z

I sync the type system and compiler back to the CoreRT repo on weekends when I have time

Since we had a public holiday in Slovakia, managed to sync the compiler a bit earlier. dotnet/corert#8122.

AntonLapounov · 2020-05-01T19:48:52Z

@davidwrighton Below are results of --gcstress 3 for pri-0 tests after the changes. Before the changes the framework could not be compiled with crossgen2.

Compilation	Crossgen	CPAOT
PASS	3987	3998
FAIL	14	3
Total	4001	4001

Execution	Crossgen	CPAOT
PASS	2765	2754
EXIT_CODE	8	18
CRASHED	0	0
TIMED_OUT	6	6
BUILD_FAILED	2	3
Total	2781	2781

AntonLapounov · 2020-05-01T20:17:47Z

@echesakovMSFT This PR includes minor changes under jit and vm. It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type. Otherwise, JIT fails later in the prolog codegen phase or the register allocation phase.

AntonLapounov · 2020-05-01T21:26:15Z

@CarolEidt Implementation of HFA/HVA property calculation in this PR is free of issues documented in #35144. We need them to be fixed so that Crossgen2 compiler and VM agree on the calling convention. One possible way of fixing that is to introduce a new enumeration for fundamental data types of homogeneous aggregates, like I did in this PR: https://github.com/dotnet/runtime/pull/35576/files#diff-e553d696b7062596f5653d5bba53d278, and use it as the return type of the getHFAType callback. I also suggested adding an assertion for its returned value: #35576 (comment).

CarolEidt · 2020-05-02T00:12:47Z

It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type.

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@AntonLapounov - I was previously thinking that fixing #35144 would require a change to the JIT/EE interface, but I believe it's the case that from a JIT perspective it doesn't really need to distinguish between an HFA of double and an HVA of SIMD8. So I think the fix just needs to be made on the vm side of the JIT/EE interface, as you've done here.

CarolEidt

The JIT changes LGTM

AntonLapounov · 2020-05-02T00:51:25Z

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@CarolEidt I would prefer someone with more JIT knowledge to do that. You are right that technically we do not have to change the getHFA API; however, I find using ELEMENT_TYPE_VALUETYPE to actually mean HfaElemKind.HFA_ELEM_SIMD16 (apparently JIT already has this enumeration) less than perfect.

janvorli · 2020-05-04T10:53:36Z

@AntonLapounov as for the timed out tests, can you please try to bump the execution timeout using the --execution-timeout-minutes r2rtest option to see if the timeouts are just taking more time or they represent hangs? You can try to bump it to e.g. 60 minutes to give it enough space.

AntonLapounov · 2020-04-28T17:15:58Z

Support ARM64 HVAs in Crossgen2.

Change FieldLayoutAlgorithm.ComputeValueTypeShapeCharacteristics method to compute the homogeneous aggregate element type and cache it in the existing field. That allows to remove all ComputeHomogeneousFloatAggregateElementType methods.
Change MetadataFieldLayoutAlgorithm.ComputeHomogeneousAggregateCharacteristic to compute HVAs in addition to HFAs.
Change CorInfoImpl.getHFAType JIT callback to handle HVAs. Note that returning ELEMENT_TYPE_VALUETYPE indicates the TYP_SIMD16 type (see Compiler::GetHfaType).
Change TypeFixupSignature.EncodeTypeLayout to handle HVAs.
Support HVAs in the ArgIterator class.
Fix TransitionBlock.OffsetFromGCRefMapPos for ARM64. R2RDump used to dump incorrect offsets.
Use TransitionBlock.OffsetFromGCRefMapPos in GCRefMapBuilder.GetCallRefMap to simplify logic.
Remove ARM64 .NET Native-specific code from TransitionBlock.cs and ArgIterator.cs files.

Minor:

Remove a redundant GetVectorSize check in MethodTable::GetHFAType.
Improve an assertion in Compiler::raUpdateRegStateForArg.
Fix comments in JIT code.

AntonLapounov · 2020-04-28T17:21:08Z

- vectorSize = pMT->GetVectorSize();
- if (vectorSize != 0)
- {
- return (vectorSize == 8) ? ELEMENT_TYPE_R8 : ELEMENT_TYPE_VALUETYPE;
- }


The next loop iteration performs this check anyway, so this one is redundant.

AntonLapounov · 2020-04-28T17:28:58Z

 }
 else
 {
+ assert(!regState->rsIsFloat);


We could miss this check due to the break below, then fail later in an unexpected way.

cc: @dotnet/jit-contrib

MichalStrehovsky · 2020-04-29T09:16:38Z

We have xUnit test coverage for these aspects in the CoreRT repo: https://github.com/dotnet/corert/blob/923eeee2fb70497fe07b74d56bc51bee029c108b/src/ILCompiler.TypeSystem/tests/ValueTypeShapeCharacteristicsTests.cs

Issue #200 tracks porting these xUnit tests to the runtime repo, but for now they only compile and run in the experimental CoreRT repo (they will run if you just build.cmd from the root of the repo). I don't know if porting them is on anyone's radar soon (it would be about figuring out how to run xUnit as part of the coreclr managed tools build). Cc @dotnet/crossgen-contrib on that.

We haven't made significant type system changes in the runtime repo yet, so there's no process for this.

I sync the type system and compiler back to the CoreRT repo on weekends when I have time. Truth to be told, when I port this over and tests stop working, I'm just going to delete the tests because I don't have that much time.

If it's not too much hassle, could you do this change in the CoreRT repo too, and adjust/add test coverage as needed? Type system bugs tend to be subtle and that's why we try to unit test as much of it as possible.

You should be able to just xcopy src\coreclr\src\tools\Common\TypeSystem to src\Common\src\TypeSystem and src\coreclr\src\tools\Common\JitInterface to src\JitInterface\src. There's about two weeks worth of diffs because I didn't sync in 2 weeks, but it should be manageable to just undo irrelevant changes.

davidwrighton

I'd like to see some effect on GCStress numbers. @janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

davidwrighton · 2020-04-29T21:10:54Z

 {
- int ofs;
-
- if (_target.Architecture == TargetArchitecture.X86)


I realize that we don't yet support Crossgen2 for x86, but is removing this special case correct? Should it instead be moved to the OffsetFromGCRefMapPos function?

This special case is already in OffsetFromGCRefMapPos overload for x86:
runtime/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TransitionBlock.cs
Lines 375 to 385 in 6ce7610
publicoverrideintOffsetFromGCRefMapPos(intpos)
{
if(pos<NumArgumentRegisters)
{
returnOffsetOfArgumentRegisters+SizeOfArgumentRegisters-(pos+1)*PointerSize;
}
else
{
returnOffsetOfArgs+(pos-NumArgumentRegisters)*PointerSize;
}
}

janvorli · 2020-04-29T21:47:19Z

@janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

Running with GC stress enabled is as easy as passing --gcstress argument to r2rtest tool (e.g. --gcstress 3). Or, if you are using runtest.cmd, then passing in gcstresslevel argument (and runcrossgen2tests to use crossgen2).

janvorli

LGTM, thank you!

MichalStrehovsky · 2020-05-01T17:25:49Z

I sync the type system and compiler back to the CoreRT repo on weekends when I have time

Since we had a public holiday in Slovakia, managed to sync the compiler a bit earlier. dotnet/corert#8122.

AntonLapounov · 2020-05-01T19:48:52Z

@davidwrighton Below are results of --gcstress 3 for pri-0 tests after the changes. Before the changes the framework could not be compiled with crossgen2.

Compilation	Crossgen	CPAOT
PASS	3987	3998
FAIL	14	3
Total	4001	4001

Execution	Crossgen	CPAOT
PASS	2765	2754
EXIT_CODE	8	18
CRASHED	0	0
TIMED_OUT	6	6
BUILD_FAILED	2	3
Total	2781	2781

AntonLapounov · 2020-05-01T20:17:47Z

@echesakovMSFT This PR includes minor changes under jit and vm. It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type. Otherwise, JIT fails later in the prolog codegen phase or the register allocation phase.

AntonLapounov · 2020-05-01T21:26:15Z

@CarolEidt Implementation of HFA/HVA property calculation in this PR is free of issues documented in #35144. We need them to be fixed so that Crossgen2 compiler and VM agree on the calling convention. One possible way of fixing that is to introduce a new enumeration for fundamental data types of homogeneous aggregates, like I did in this PR: https://github.com/dotnet/runtime/pull/35576/files#diff-e553d696b7062596f5653d5bba53d278, and use it as the return type of the getHFAType callback. I also suggested adding an assertion for its returned value: #35576 (comment).

CarolEidt · 2020-05-02T00:12:47Z

It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type.

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@AntonLapounov - I was previously thinking that fixing #35144 would require a change to the JIT/EE interface, but I believe it's the case that from a JIT perspective it doesn't really need to distinguish between an HFA of double and an HVA of SIMD8. So I think the fix just needs to be made on the vm side of the JIT/EE interface, as you've done here.

CarolEidt

The JIT changes LGTM

AntonLapounov · 2020-05-02T00:51:25Z

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@CarolEidt I would prefer someone with more JIT knowledge to do that. You are right that technically we do not have to change the getHFA API; however, I find using ELEMENT_TYPE_VALUETYPE to actually mean HfaElemKind.HFA_ELEM_SIMD16 (apparently JIT already has this enumeration) less than perfect.

janvorli · 2020-05-04T10:53:36Z

@AntonLapounov as for the timed out tests, can you please try to bump the execution timeout using the --execution-timeout-minutes r2rtest option to see if the timeouts are just taking more time or they represent hangs? You can try to bump it to e.g. 60 minutes to give it enough space.

AntonLapounov · 2020-04-28T17:15:58Z

Support ARM64 HVAs in Crossgen2.

Change FieldLayoutAlgorithm.ComputeValueTypeShapeCharacteristics method to compute the homogeneous aggregate element type and cache it in the existing field. That allows to remove all ComputeHomogeneousFloatAggregateElementType methods.
Change MetadataFieldLayoutAlgorithm.ComputeHomogeneousAggregateCharacteristic to compute HVAs in addition to HFAs.
Change CorInfoImpl.getHFAType JIT callback to handle HVAs. Note that returning ELEMENT_TYPE_VALUETYPE indicates the TYP_SIMD16 type (see Compiler::GetHfaType).
Change TypeFixupSignature.EncodeTypeLayout to handle HVAs.
Support HVAs in the ArgIterator class.
Fix TransitionBlock.OffsetFromGCRefMapPos for ARM64. R2RDump used to dump incorrect offsets.
Use TransitionBlock.OffsetFromGCRefMapPos in GCRefMapBuilder.GetCallRefMap to simplify logic.
Remove ARM64 .NET Native-specific code from TransitionBlock.cs and ArgIterator.cs files.

Minor:

Remove a redundant GetVectorSize check in MethodTable::GetHFAType.
Improve an assertion in Compiler::raUpdateRegStateForArg.
Fix comments in JIT code.

AntonLapounov · 2020-04-28T17:21:08Z

- vectorSize = pMT->GetVectorSize();
- if (vectorSize != 0)
- {
- return (vectorSize == 8) ? ELEMENT_TYPE_R8 : ELEMENT_TYPE_VALUETYPE;
- }


The next loop iteration performs this check anyway, so this one is redundant.

AntonLapounov · 2020-04-28T17:28:58Z

 }
 else
 {
+ assert(!regState->rsIsFloat);


We could miss this check due to the break below, then fail later in an unexpected way.

cc: @dotnet/jit-contrib

MichalStrehovsky · 2020-04-29T09:16:38Z

We have xUnit test coverage for these aspects in the CoreRT repo: https://github.com/dotnet/corert/blob/923eeee2fb70497fe07b74d56bc51bee029c108b/src/ILCompiler.TypeSystem/tests/ValueTypeShapeCharacteristicsTests.cs

Issue #200 tracks porting these xUnit tests to the runtime repo, but for now they only compile and run in the experimental CoreRT repo (they will run if you just build.cmd from the root of the repo). I don't know if porting them is on anyone's radar soon (it would be about figuring out how to run xUnit as part of the coreclr managed tools build). Cc @dotnet/crossgen-contrib on that.

We haven't made significant type system changes in the runtime repo yet, so there's no process for this.

I sync the type system and compiler back to the CoreRT repo on weekends when I have time. Truth to be told, when I port this over and tests stop working, I'm just going to delete the tests because I don't have that much time.

If it's not too much hassle, could you do this change in the CoreRT repo too, and adjust/add test coverage as needed? Type system bugs tend to be subtle and that's why we try to unit test as much of it as possible.

You should be able to just xcopy src\coreclr\src\tools\Common\TypeSystem to src\Common\src\TypeSystem and src\coreclr\src\tools\Common\JitInterface to src\JitInterface\src. There's about two weeks worth of diffs because I didn't sync in 2 weeks, but it should be manageable to just undo irrelevant changes.

davidwrighton

I'd like to see some effect on GCStress numbers. @janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

davidwrighton · 2020-04-29T21:10:54Z

 {
- int ofs;
-
- if (_target.Architecture == TargetArchitecture.X86)


I realize that we don't yet support Crossgen2 for x86, but is removing this special case correct? Should it instead be moved to the OffsetFromGCRefMapPos function?

This special case is already in OffsetFromGCRefMapPos overload for x86:
runtime/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TransitionBlock.cs
Lines 375 to 385 in 6ce7610
publicoverrideintOffsetFromGCRefMapPos(intpos)
{
if(pos<NumArgumentRegisters)
{
returnOffsetOfArgumentRegisters+SizeOfArgumentRegisters-(pos+1)*PointerSize;
}
else
{
returnOffsetOfArgs+(pos-NumArgumentRegisters)*PointerSize;
}
}

janvorli · 2020-04-29T21:47:19Z

@janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

Running with GC stress enabled is as easy as passing --gcstress argument to r2rtest tool (e.g. --gcstress 3). Or, if you are using runtest.cmd, then passing in gcstresslevel argument (and runcrossgen2tests to use crossgen2).

janvorli

LGTM, thank you!

MichalStrehovsky · 2020-05-01T17:25:49Z

I sync the type system and compiler back to the CoreRT repo on weekends when I have time

Since we had a public holiday in Slovakia, managed to sync the compiler a bit earlier. dotnet/corert#8122.

AntonLapounov · 2020-05-01T19:48:52Z

@davidwrighton Below are results of --gcstress 3 for pri-0 tests after the changes. Before the changes the framework could not be compiled with crossgen2.

Compilation	Crossgen	CPAOT
PASS	3987	3998
FAIL	14	3
Total	4001	4001

Execution	Crossgen	CPAOT
PASS	2765	2754
EXIT_CODE	8	18
CRASHED	0	0
TIMED_OUT	6	6
BUILD_FAILED	2	3
Total	2781	2781

AntonLapounov · 2020-05-01T20:17:47Z

@echesakovMSFT This PR includes minor changes under jit and vm. It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type. Otherwise, JIT fails later in the prolog codegen phase or the register allocation phase.

AntonLapounov · 2020-05-01T21:26:15Z

@CarolEidt Implementation of HFA/HVA property calculation in this PR is free of issues documented in #35144. We need them to be fixed so that Crossgen2 compiler and VM agree on the calling convention. One possible way of fixing that is to introduce a new enumeration for fundamental data types of homogeneous aggregates, like I did in this PR: https://github.com/dotnet/runtime/pull/35576/files#diff-e553d696b7062596f5653d5bba53d278, and use it as the return type of the getHFAType callback. I also suggested adding an assertion for its returned value: #35576 (comment).

CarolEidt · 2020-05-02T00:12:47Z

It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type.

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@AntonLapounov - I was previously thinking that fixing #35144 would require a change to the JIT/EE interface, but I believe it's the case that from a JIT perspective it doesn't really need to distinguish between an HFA of double and an HVA of SIMD8. So I think the fix just needs to be made on the vm side of the JIT/EE interface, as you've done here.

CarolEidt

The JIT changes LGTM

AntonLapounov · 2020-05-02T00:51:25Z

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@CarolEidt I would prefer someone with more JIT knowledge to do that. You are right that technically we do not have to change the getHFA API; however, I find using ELEMENT_TYPE_VALUETYPE to actually mean HfaElemKind.HFA_ELEM_SIMD16 (apparently JIT already has this enumeration) less than perfect.

janvorli · 2020-05-04T10:53:36Z

@AntonLapounov as for the timed out tests, can you please try to bump the execution timeout using the --execution-timeout-minutes r2rtest option to see if the timeouts are just taking more time or they represent hangs? You can try to bump it to e.g. 60 minutes to give it enough space.

AntonLapounov · 2020-04-28T17:15:58Z

Support ARM64 HVAs in Crossgen2.

Change FieldLayoutAlgorithm.ComputeValueTypeShapeCharacteristics method to compute the homogeneous aggregate element type and cache it in the existing field. That allows to remove all ComputeHomogeneousFloatAggregateElementType methods.
Change MetadataFieldLayoutAlgorithm.ComputeHomogeneousAggregateCharacteristic to compute HVAs in addition to HFAs.
Change CorInfoImpl.getHFAType JIT callback to handle HVAs. Note that returning ELEMENT_TYPE_VALUETYPE indicates the TYP_SIMD16 type (see Compiler::GetHfaType).
Change TypeFixupSignature.EncodeTypeLayout to handle HVAs.
Support HVAs in the ArgIterator class.
Fix TransitionBlock.OffsetFromGCRefMapPos for ARM64. R2RDump used to dump incorrect offsets.
Use TransitionBlock.OffsetFromGCRefMapPos in GCRefMapBuilder.GetCallRefMap to simplify logic.
Remove ARM64 .NET Native-specific code from TransitionBlock.cs and ArgIterator.cs files.

Minor:

Remove a redundant GetVectorSize check in MethodTable::GetHFAType.
Improve an assertion in Compiler::raUpdateRegStateForArg.
Fix comments in JIT code.

AntonLapounov · 2020-04-28T17:21:08Z

- vectorSize = pMT->GetVectorSize();
- if (vectorSize != 0)
- {
- return (vectorSize == 8) ? ELEMENT_TYPE_R8 : ELEMENT_TYPE_VALUETYPE;
- }


The next loop iteration performs this check anyway, so this one is redundant.

AntonLapounov · 2020-04-28T17:28:58Z

 }
 else
 {
+ assert(!regState->rsIsFloat);


We could miss this check due to the break below, then fail later in an unexpected way.

cc: @dotnet/jit-contrib

MichalStrehovsky · 2020-04-29T09:16:38Z

We have xUnit test coverage for these aspects in the CoreRT repo: https://github.com/dotnet/corert/blob/923eeee2fb70497fe07b74d56bc51bee029c108b/src/ILCompiler.TypeSystem/tests/ValueTypeShapeCharacteristicsTests.cs

Issue #200 tracks porting these xUnit tests to the runtime repo, but for now they only compile and run in the experimental CoreRT repo (they will run if you just build.cmd from the root of the repo). I don't know if porting them is on anyone's radar soon (it would be about figuring out how to run xUnit as part of the coreclr managed tools build). Cc @dotnet/crossgen-contrib on that.

We haven't made significant type system changes in the runtime repo yet, so there's no process for this.

I sync the type system and compiler back to the CoreRT repo on weekends when I have time. Truth to be told, when I port this over and tests stop working, I'm just going to delete the tests because I don't have that much time.

If it's not too much hassle, could you do this change in the CoreRT repo too, and adjust/add test coverage as needed? Type system bugs tend to be subtle and that's why we try to unit test as much of it as possible.

You should be able to just xcopy src\coreclr\src\tools\Common\TypeSystem to src\Common\src\TypeSystem and src\coreclr\src\tools\Common\JitInterface to src\JitInterface\src. There's about two weeks worth of diffs because I didn't sync in 2 weeks, but it should be manageable to just undo irrelevant changes.

davidwrighton

I'd like to see some effect on GCStress numbers. @janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

davidwrighton · 2020-04-29T21:10:54Z

 {
- int ofs;
-
- if (_target.Architecture == TargetArchitecture.X86)


I realize that we don't yet support Crossgen2 for x86, but is removing this special case correct? Should it instead be moved to the OffsetFromGCRefMapPos function?

This special case is already in OffsetFromGCRefMapPos overload for x86:
runtime/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TransitionBlock.cs
Lines 375 to 385 in 6ce7610
publicoverrideintOffsetFromGCRefMapPos(intpos)
{
if(pos<NumArgumentRegisters)
{
returnOffsetOfArgumentRegisters+SizeOfArgumentRegisters-(pos+1)*PointerSize;
}
else
{
returnOffsetOfArgs+(pos-NumArgumentRegisters)*PointerSize;
}
}

janvorli · 2020-04-29T21:47:19Z

@janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

Running with GC stress enabled is as easy as passing --gcstress argument to r2rtest tool (e.g. --gcstress 3). Or, if you are using runtest.cmd, then passing in gcstresslevel argument (and runcrossgen2tests to use crossgen2).

janvorli

LGTM, thank you!

MichalStrehovsky · 2020-05-01T17:25:49Z

I sync the type system and compiler back to the CoreRT repo on weekends when I have time

Since we had a public holiday in Slovakia, managed to sync the compiler a bit earlier. dotnet/corert#8122.

AntonLapounov · 2020-05-01T19:48:52Z

@davidwrighton Below are results of --gcstress 3 for pri-0 tests after the changes. Before the changes the framework could not be compiled with crossgen2.

Compilation	Crossgen	CPAOT
PASS	3987	3998
FAIL	14	3
Total	4001	4001

Execution	Crossgen	CPAOT
PASS	2765	2754
EXIT_CODE	8	18
CRASHED	0	0
TIMED_OUT	6	6
BUILD_FAILED	2	3
Total	2781	2781

AntonLapounov · 2020-05-01T20:17:47Z

@echesakovMSFT This PR includes minor changes under jit and vm. It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type. Otherwise, JIT fails later in the prolog codegen phase or the register allocation phase.

AntonLapounov · 2020-05-01T21:26:15Z

@CarolEidt Implementation of HFA/HVA property calculation in this PR is free of issues documented in #35144. We need them to be fixed so that Crossgen2 compiler and VM agree on the calling convention. One possible way of fixing that is to introduce a new enumeration for fundamental data types of homogeneous aggregates, like I did in this PR: https://github.com/dotnet/runtime/pull/35576/files#diff-e553d696b7062596f5653d5bba53d278, and use it as the return type of the getHFAType callback. I also suggested adding an assertion for its returned value: #35576 (comment).

CarolEidt · 2020-05-02T00:12:47Z

It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type.

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@AntonLapounov - I was previously thinking that fixing #35144 would require a change to the JIT/EE interface, but I believe it's the case that from a JIT perspective it doesn't really need to distinguish between an HFA of double and an HVA of SIMD8. So I think the fix just needs to be made on the vm side of the JIT/EE interface, as you've done here.

CarolEidt

The JIT changes LGTM

AntonLapounov · 2020-05-02T00:51:25Z

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@CarolEidt I would prefer someone with more JIT knowledge to do that. You are right that technically we do not have to change the getHFA API; however, I find using ELEMENT_TYPE_VALUETYPE to actually mean HfaElemKind.HFA_ELEM_SIMD16 (apparently JIT already has this enumeration) less than perfect.

janvorli · 2020-05-04T10:53:36Z

@AntonLapounov as for the timed out tests, can you please try to bump the execution timeout using the --execution-timeout-minutes r2rtest option to see if the timeouts are just taking more time or they represent hangs? You can try to bump it to e.g. 60 minutes to give it enough space.

AntonLapounov · 2020-04-28T17:15:58Z

Support ARM64 HVAs in Crossgen2.

Change FieldLayoutAlgorithm.ComputeValueTypeShapeCharacteristics method to compute the homogeneous aggregate element type and cache it in the existing field. That allows to remove all ComputeHomogeneousFloatAggregateElementType methods.
Change MetadataFieldLayoutAlgorithm.ComputeHomogeneousAggregateCharacteristic to compute HVAs in addition to HFAs.
Change CorInfoImpl.getHFAType JIT callback to handle HVAs. Note that returning ELEMENT_TYPE_VALUETYPE indicates the TYP_SIMD16 type (see Compiler::GetHfaType).
Change TypeFixupSignature.EncodeTypeLayout to handle HVAs.
Support HVAs in the ArgIterator class.
Fix TransitionBlock.OffsetFromGCRefMapPos for ARM64. R2RDump used to dump incorrect offsets.
Use TransitionBlock.OffsetFromGCRefMapPos in GCRefMapBuilder.GetCallRefMap to simplify logic.
Remove ARM64 .NET Native-specific code from TransitionBlock.cs and ArgIterator.cs files.

Minor:

Remove a redundant GetVectorSize check in MethodTable::GetHFAType.
Improve an assertion in Compiler::raUpdateRegStateForArg.
Fix comments in JIT code.

AntonLapounov · 2020-04-28T17:21:08Z

- vectorSize = pMT->GetVectorSize();
- if (vectorSize != 0)
- {
- return (vectorSize == 8) ? ELEMENT_TYPE_R8 : ELEMENT_TYPE_VALUETYPE;
- }


The next loop iteration performs this check anyway, so this one is redundant.

AntonLapounov · 2020-04-28T17:28:58Z

 }
 else
 {
+ assert(!regState->rsIsFloat);


We could miss this check due to the break below, then fail later in an unexpected way.

cc: @dotnet/jit-contrib

MichalStrehovsky · 2020-04-29T09:16:38Z

We have xUnit test coverage for these aspects in the CoreRT repo: https://github.com/dotnet/corert/blob/923eeee2fb70497fe07b74d56bc51bee029c108b/src/ILCompiler.TypeSystem/tests/ValueTypeShapeCharacteristicsTests.cs

Issue #200 tracks porting these xUnit tests to the runtime repo, but for now they only compile and run in the experimental CoreRT repo (they will run if you just build.cmd from the root of the repo). I don't know if porting them is on anyone's radar soon (it would be about figuring out how to run xUnit as part of the coreclr managed tools build). Cc @dotnet/crossgen-contrib on that.

We haven't made significant type system changes in the runtime repo yet, so there's no process for this.

I sync the type system and compiler back to the CoreRT repo on weekends when I have time. Truth to be told, when I port this over and tests stop working, I'm just going to delete the tests because I don't have that much time.

If it's not too much hassle, could you do this change in the CoreRT repo too, and adjust/add test coverage as needed? Type system bugs tend to be subtle and that's why we try to unit test as much of it as possible.

You should be able to just xcopy src\coreclr\src\tools\Common\TypeSystem to src\Common\src\TypeSystem and src\coreclr\src\tools\Common\JitInterface to src\JitInterface\src. There's about two weeks worth of diffs because I didn't sync in 2 weeks, but it should be manageable to just undo irrelevant changes.

davidwrighton

I'd like to see some effect on GCStress numbers. @janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

davidwrighton · 2020-04-29T21:10:54Z

 {
- int ofs;
-
- if (_target.Architecture == TargetArchitecture.X86)


I realize that we don't yet support Crossgen2 for x86, but is removing this special case correct? Should it instead be moved to the OffsetFromGCRefMapPos function?

This special case is already in OffsetFromGCRefMapPos overload for x86:
runtime/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TransitionBlock.cs
Lines 375 to 385 in 6ce7610
publicoverrideintOffsetFromGCRefMapPos(intpos)
{
if(pos<NumArgumentRegisters)
{
returnOffsetOfArgumentRegisters+SizeOfArgumentRegisters-(pos+1)*PointerSize;
}
else
{
returnOffsetOfArgs+(pos-NumArgumentRegisters)*PointerSize;
}
}

janvorli · 2020-04-29T21:47:19Z

@janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

Running with GC stress enabled is as easy as passing --gcstress argument to r2rtest tool (e.g. --gcstress 3). Or, if you are using runtest.cmd, then passing in gcstresslevel argument (and runcrossgen2tests to use crossgen2).

janvorli

LGTM, thank you!

MichalStrehovsky · 2020-05-01T17:25:49Z

I sync the type system and compiler back to the CoreRT repo on weekends when I have time

Since we had a public holiday in Slovakia, managed to sync the compiler a bit earlier. dotnet/corert#8122.

AntonLapounov · 2020-05-01T19:48:52Z

@davidwrighton Below are results of --gcstress 3 for pri-0 tests after the changes. Before the changes the framework could not be compiled with crossgen2.

Compilation	Crossgen	CPAOT
PASS	3987	3998
FAIL	14	3
Total	4001	4001

Execution	Crossgen	CPAOT
PASS	2765	2754
EXIT_CODE	8	18
CRASHED	0	0
TIMED_OUT	6	6
BUILD_FAILED	2	3
Total	2781	2781

AntonLapounov · 2020-05-01T20:17:47Z

@echesakovMSFT This PR includes minor changes under jit and vm. It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type. Otherwise, JIT fails later in the prolog codegen phase or the register allocation phase.

AntonLapounov · 2020-05-01T21:26:15Z

@CarolEidt Implementation of HFA/HVA property calculation in this PR is free of issues documented in #35144. We need them to be fixed so that Crossgen2 compiler and VM agree on the calling convention. One possible way of fixing that is to introduce a new enumeration for fundamental data types of homogeneous aggregates, like I did in this PR: https://github.com/dotnet/runtime/pull/35576/files#diff-e553d696b7062596f5653d5bba53d278, and use it as the return type of the getHFAType callback. I also suggested adding an assertion for its returned value: #35576 (comment).

CarolEidt · 2020-05-02T00:12:47Z

It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type.

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@AntonLapounov - I was previously thinking that fixing #35144 would require a change to the JIT/EE interface, but I believe it's the case that from a JIT perspective it doesn't really need to distinguish between an HFA of double and an HVA of SIMD8. So I think the fix just needs to be made on the vm side of the JIT/EE interface, as you've done here.

CarolEidt

The JIT changes LGTM

AntonLapounov · 2020-05-02T00:51:25Z

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@CarolEidt I would prefer someone with more JIT knowledge to do that. You are right that technically we do not have to change the getHFA API; however, I find using ELEMENT_TYPE_VALUETYPE to actually mean HfaElemKind.HFA_ELEM_SIMD16 (apparently JIT already has this enumeration) less than perfect.

janvorli · 2020-05-04T10:53:36Z

@AntonLapounov as for the timed out tests, can you please try to bump the execution timeout using the --execution-timeout-minutes r2rtest option to see if the timeouts are just taking more time or they represent hangs? You can try to bump it to e.g. 60 minutes to give it enough space.

AntonLapounov · 2020-04-28T17:15:58Z

Support ARM64 HVAs in Crossgen2.

Change FieldLayoutAlgorithm.ComputeValueTypeShapeCharacteristics method to compute the homogeneous aggregate element type and cache it in the existing field. That allows to remove all ComputeHomogeneousFloatAggregateElementType methods.
Change MetadataFieldLayoutAlgorithm.ComputeHomogeneousAggregateCharacteristic to compute HVAs in addition to HFAs.
Change CorInfoImpl.getHFAType JIT callback to handle HVAs. Note that returning ELEMENT_TYPE_VALUETYPE indicates the TYP_SIMD16 type (see Compiler::GetHfaType).
Change TypeFixupSignature.EncodeTypeLayout to handle HVAs.
Support HVAs in the ArgIterator class.
Fix TransitionBlock.OffsetFromGCRefMapPos for ARM64. R2RDump used to dump incorrect offsets.
Use TransitionBlock.OffsetFromGCRefMapPos in GCRefMapBuilder.GetCallRefMap to simplify logic.
Remove ARM64 .NET Native-specific code from TransitionBlock.cs and ArgIterator.cs files.

Minor:

Remove a redundant GetVectorSize check in MethodTable::GetHFAType.
Improve an assertion in Compiler::raUpdateRegStateForArg.
Fix comments in JIT code.

AntonLapounov · 2020-04-28T17:21:08Z

- vectorSize = pMT->GetVectorSize();
- if (vectorSize != 0)
- {
- return (vectorSize == 8) ? ELEMENT_TYPE_R8 : ELEMENT_TYPE_VALUETYPE;
- }


The next loop iteration performs this check anyway, so this one is redundant.

AntonLapounov · 2020-04-28T17:28:58Z

 }
 else
 {
+ assert(!regState->rsIsFloat);


We could miss this check due to the break below, then fail later in an unexpected way.

cc: @dotnet/jit-contrib

MichalStrehovsky · 2020-04-29T09:16:38Z

We have xUnit test coverage for these aspects in the CoreRT repo: https://github.com/dotnet/corert/blob/923eeee2fb70497fe07b74d56bc51bee029c108b/src/ILCompiler.TypeSystem/tests/ValueTypeShapeCharacteristicsTests.cs

Issue #200 tracks porting these xUnit tests to the runtime repo, but for now they only compile and run in the experimental CoreRT repo (they will run if you just build.cmd from the root of the repo). I don't know if porting them is on anyone's radar soon (it would be about figuring out how to run xUnit as part of the coreclr managed tools build). Cc @dotnet/crossgen-contrib on that.

We haven't made significant type system changes in the runtime repo yet, so there's no process for this.

I sync the type system and compiler back to the CoreRT repo on weekends when I have time. Truth to be told, when I port this over and tests stop working, I'm just going to delete the tests because I don't have that much time.

If it's not too much hassle, could you do this change in the CoreRT repo too, and adjust/add test coverage as needed? Type system bugs tend to be subtle and that's why we try to unit test as much of it as possible.

You should be able to just xcopy src\coreclr\src\tools\Common\TypeSystem to src\Common\src\TypeSystem and src\coreclr\src\tools\Common\JitInterface to src\JitInterface\src. There's about two weeks worth of diffs because I didn't sync in 2 weeks, but it should be manageable to just undo irrelevant changes.

davidwrighton

I'd like to see some effect on GCStress numbers. @janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

davidwrighton · 2020-04-29T21:10:54Z

 {
- int ofs;
-
- if (_target.Architecture == TargetArchitecture.X86)


I realize that we don't yet support Crossgen2 for x86, but is removing this special case correct? Should it instead be moved to the OffsetFromGCRefMapPos function?

This special case is already in OffsetFromGCRefMapPos overload for x86:
runtime/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TransitionBlock.cs
Lines 375 to 385 in 6ce7610
publicoverrideintOffsetFromGCRefMapPos(intpos)
{
if(pos<NumArgumentRegisters)
{
returnOffsetOfArgumentRegisters+SizeOfArgumentRegisters-(pos+1)*PointerSize;
}
else
{
returnOffsetOfArgs+(pos-NumArgumentRegisters)*PointerSize;
}
}

janvorli · 2020-04-29T21:47:19Z

@janvorli has run GCStress for crossgen2 images on X64, and I imagine he has some instructions for doing so that you could adapt to Arm64.

Running with GC stress enabled is as easy as passing --gcstress argument to r2rtest tool (e.g. --gcstress 3). Or, if you are using runtest.cmd, then passing in gcstresslevel argument (and runcrossgen2tests to use crossgen2).

janvorli

LGTM, thank you!

MichalStrehovsky · 2020-05-01T17:25:49Z

I sync the type system and compiler back to the CoreRT repo on weekends when I have time

Since we had a public holiday in Slovakia, managed to sync the compiler a bit earlier. dotnet/corert#8122.

AntonLapounov · 2020-05-01T19:48:52Z

@davidwrighton Below are results of --gcstress 3 for pri-0 tests after the changes. Before the changes the framework could not be compiled with crossgen2.

Compilation	Crossgen	CPAOT
PASS	3987	3998
FAIL	14	3
Total	4001	4001

Execution	Crossgen	CPAOT
PASS	2765	2754
EXIT_CODE	8	18
CRASHED	0	0
TIMED_OUT	6	6
BUILD_FAILED	2	3
Total	2781	2781

AntonLapounov · 2020-05-01T20:17:47Z

@echesakovMSFT This PR includes minor changes under jit and vm. It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type. Otherwise, JIT fails later in the prolog codegen phase or the register allocation phase.

AntonLapounov · 2020-05-01T21:26:15Z

@CarolEidt Implementation of HFA/HVA property calculation in this PR is free of issues documented in #35144. We need them to be fixed so that Crossgen2 compiler and VM agree on the calling convention. One possible way of fixing that is to introduce a new enumeration for fundamental data types of homogeneous aggregates, like I did in this PR: https://github.com/dotnet/runtime/pull/35576/files#diff-e553d696b7062596f5653d5bba53d278, and use it as the return type of the getHFAType callback. I also suggested adding an assertion for its returned value: #35576 (comment).

CarolEidt · 2020-05-02T00:12:47Z

It would be good to add an assert that if JIT recognizes some type as SIMD (see types in Compiler::getBaseTypeAndSizeOfSIMDType), then the GetHFAType callback returns a valid HFA/HVA type.

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@AntonLapounov - I was previously thinking that fixing #35144 would require a change to the JIT/EE interface, but I believe it's the case that from a JIT perspective it doesn't really need to distinguish between an HFA of double and an HVA of SIMD8. So I think the fix just needs to be made on the vm side of the JIT/EE interface, as you've done here.

CarolEidt

The JIT changes LGTM

AntonLapounov · 2020-05-02T00:51:25Z

Did you want to add that to this PR, or perhaps file an issue for the JIT to do that?

@CarolEidt I would prefer someone with more JIT knowledge to do that. You are right that technically we do not have to change the getHFA API; however, I find using ELEMENT_TYPE_VALUETYPE to actually mean HfaElemKind.HFA_ELEM_SIMD16 (apparently JIT already has this enumeration) less than perfect.

janvorli · 2020-05-04T10:53:36Z

@AntonLapounov as for the timed out tests, can you please try to bump the execution timeout using the --execution-timeout-minutes r2rtest option to see if the timeouts are just taking more time or they represent hangs? You can try to bump it to e.g. 60 minutes to give it enough space.

Improve HFA/HVA handling in CrossGen2.
aa1f7b8

Dotnet-GitSync-Bot added the area-ReadyToRun label Apr 28, 2020

AntonLapounov self-assigned this Apr 28, 2020

AntonLapounov added the arch-arm64 label Apr 28, 2020

AntonLapounov commented Apr 28, 2020
View reviewed changes

AntonLapounov requested review from davidwrighton and janvorli April 28, 2020 17:57

davidwrighton reviewed Apr 29, 2020
View reviewed changes

janvorli approved these changes Apr 30, 2020
View reviewed changes

Additional fixes.
e6b700d

CarolEidt approved these changes May 2, 2020
View reviewed changes

AntonLapounov merged commit 5893741 into dotnet:masterMay 2, 2020

AntonLapounov deleted the Arm64Hva branch May 2, 2020 02:56

AntonLapounov mentioned this pull request May 5, 2020
Fix buffer overrun in JIT for Vector256<T> types on ARM64 #35864
Merged

AntonLapounov mentioned this pull request Jun 9, 2020
Port changes related to homogeneous aggregate recognition dotnet/corert#8198
Merged

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Improve HFA/HVA handling in CrossGen2.
aa1f7b8

Dotnet-GitSync-Bot added the area-ReadyToRun label Apr 28, 2020

AntonLapounov self-assigned this Apr 28, 2020

AntonLapounov added the arch-arm64 label Apr 28, 2020

AntonLapounov commented Apr 28, 2020
View reviewed changes

AntonLapounov requested review from davidwrighton and janvorli April 28, 2020 17:57

davidwrighton reviewed Apr 29, 2020
View reviewed changes

janvorli approved these changes Apr 30, 2020
View reviewed changes

Additional fixes.
e6b700d

CarolEidt approved these changes May 2, 2020
View reviewed changes

AntonLapounov merged commit 5893741 into dotnet:masterMay 2, 2020

AntonLapounov deleted the Arm64Hva branch May 2, 2020 02:56

AntonLapounov mentioned this pull request May 5, 2020
Fix buffer overrun in JIT for Vector256<T> types on ARM64 #35864
Merged

AntonLapounov mentioned this pull request Jun 9, 2020
Port changes related to homogeneous aggregate recognition dotnet/corert#8198
Merged

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Improve HFA/HVA handling in CrossGen2.
aa1f7b8

Dotnet-GitSync-Bot added the area-ReadyToRun label Apr 28, 2020

AntonLapounov self-assigned this Apr 28, 2020

AntonLapounov added the arch-arm64 label Apr 28, 2020

AntonLapounov commented Apr 28, 2020
View reviewed changes

AntonLapounov requested review from davidwrighton and janvorli April 28, 2020 17:57

davidwrighton reviewed Apr 29, 2020
View reviewed changes

janvorli approved these changes Apr 30, 2020
View reviewed changes

Additional fixes.
e6b700d

CarolEidt approved these changes May 2, 2020
View reviewed changes

AntonLapounov merged commit 5893741 into dotnet:masterMay 2, 2020

AntonLapounov deleted the Arm64Hva branch May 2, 2020 02:56

AntonLapounov mentioned this pull request May 5, 2020
Fix buffer overrun in JIT for Vector256<T> types on ARM64 #35864
Merged

AntonLapounov mentioned this pull request Jun 9, 2020
Port changes related to homogeneous aggregate recognition dotnet/corert#8198
Merged

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Improve HFA/HVA handling in CrossGen2.
aa1f7b8

Dotnet-GitSync-Bot added the area-ReadyToRun label Apr 28, 2020

AntonLapounov self-assigned this Apr 28, 2020

AntonLapounov added the arch-arm64 label Apr 28, 2020

AntonLapounov commented Apr 28, 2020
View reviewed changes

AntonLapounov requested review from davidwrighton and janvorli April 28, 2020 17:57

davidwrighton reviewed Apr 29, 2020
View reviewed changes

janvorli approved these changes Apr 30, 2020
View reviewed changes

Additional fixes.
e6b700d

CarolEidt approved these changes May 2, 2020
View reviewed changes

AntonLapounov merged commit 5893741 into dotnet:masterMay 2, 2020

AntonLapounov deleted the Arm64Hva branch May 2, 2020 02:56

AntonLapounov mentioned this pull request May 5, 2020
Fix buffer overrun in JIT for Vector256<T> types on ARM64 #35864
Merged

AntonLapounov mentioned this pull request Jun 9, 2020
Port changes related to homogeneous aggregate recognition dotnet/corert#8198
Merged

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Improve HFA/HVA handling in CrossGen2.
aa1f7b8

Dotnet-GitSync-Bot added the area-ReadyToRun label Apr 28, 2020

AntonLapounov self-assigned this Apr 28, 2020

AntonLapounov added the arch-arm64 label Apr 28, 2020

AntonLapounov commented Apr 28, 2020
View reviewed changes

AntonLapounov requested review from davidwrighton and janvorli April 28, 2020 17:57

davidwrighton reviewed Apr 29, 2020
View reviewed changes

janvorli approved these changes Apr 30, 2020
View reviewed changes

Additional fixes.
e6b700d

CarolEidt approved these changes May 2, 2020
View reviewed changes

AntonLapounov merged commit 5893741 into dotnet:masterMay 2, 2020

AntonLapounov deleted the Arm64Hva branch May 2, 2020 02:56

AntonLapounov mentioned this pull request May 5, 2020
Fix buffer overrun in JIT for Vector256<T> types on ARM64 #35864
Merged

AntonLapounov mentioned this pull request Jun 9, 2020
Port changes related to homogeneous aggregate recognition dotnet/corert#8198
Merged

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Improve HFA/HVA handling in CrossGen2.
aa1f7b8

Dotnet-GitSync-Bot added the area-ReadyToRun label Apr 28, 2020

AntonLapounov self-assigned this Apr 28, 2020

AntonLapounov added the arch-arm64 label Apr 28, 2020

AntonLapounov commented Apr 28, 2020
View reviewed changes

AntonLapounov requested review from davidwrighton and janvorli April 28, 2020 17:57

davidwrighton reviewed Apr 29, 2020
View reviewed changes

janvorli approved these changes Apr 30, 2020
View reviewed changes

Additional fixes.
e6b700d

CarolEidt approved these changes May 2, 2020
View reviewed changes

AntonLapounov merged commit 5893741 into dotnet:masterMay 2, 2020

AntonLapounov deleted the Arm64Hva branch May 2, 2020 02:56

AntonLapounov mentioned this pull request May 5, 2020
Fix buffer overrun in JIT for Vector256<T> types on ARM64 #35864
Merged

AntonLapounov mentioned this pull request Jun 9, 2020
Port changes related to homogeneous aggregate recognition dotnet/corert#8198
Merged

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Improve HFA/HVA handling in CrossGen2.
aa1f7b8

Dotnet-GitSync-Bot added the area-ReadyToRun label Apr 28, 2020

AntonLapounov self-assigned this Apr 28, 2020

AntonLapounov added the arch-arm64 label Apr 28, 2020

AntonLapounov commented Apr 28, 2020
View reviewed changes

AntonLapounov requested review from davidwrighton and janvorli April 28, 2020 17:57

davidwrighton reviewed Apr 29, 2020
View reviewed changes

janvorli approved these changes Apr 30, 2020
View reviewed changes

Additional fixes.
e6b700d

CarolEidt approved these changes May 2, 2020
View reviewed changes

AntonLapounov merged commit 5893741 into dotnet:masterMay 2, 2020

AntonLapounov deleted the Arm64Hva branch May 2, 2020 02:56

AntonLapounov mentioned this pull request May 5, 2020
Fix buffer overrun in JIT for Vector256<T> types on ARM64 #35864
Merged

AntonLapounov mentioned this pull request Jun 9, 2020
Port changes related to homogeneous aggregate recognition dotnet/corert#8198
Merged

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Improve HFA/HVA handling in CrossGen2.
aa1f7b8

Dotnet-GitSync-Bot added the area-ReadyToRun label Apr 28, 2020

AntonLapounov self-assigned this Apr 28, 2020

AntonLapounov added the arch-arm64 label Apr 28, 2020

AntonLapounov commented Apr 28, 2020
View reviewed changes

AntonLapounov requested review from davidwrighton and janvorli April 28, 2020 17:57

davidwrighton reviewed Apr 29, 2020
View reviewed changes

janvorli approved these changes Apr 30, 2020
View reviewed changes

Additional fixes.
e6b700d

CarolEidt approved these changes May 2, 2020
View reviewed changes

AntonLapounov merged commit 5893741 into dotnet:masterMay 2, 2020

AntonLapounov deleted the Arm64Hva branch May 2, 2020 02:56

AntonLapounov mentioned this pull request May 5, 2020
Fix buffer overrun in JIT for Vector256<T> types on ARM64 #35864
Merged

AntonLapounov mentioned this pull request Jun 9, 2020
Port changes related to homogeneous aggregate recognition dotnet/corert#8198
Merged

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

	publicoverrideintOffsetFromGCRefMapPos(intpos)
	{
	if(pos<NumArgumentRegisters)
	{
	returnOffsetOfArgumentRegisters+SizeOfArgumentRegisters-(pos+1)*PointerSize;
	}
	else
	{
	returnOffsetOfArgs+(pos-NumArgumentRegisters)*PointerSize;
	}
	}

Uh oh!

Conversation

AntonLapounov commented Apr 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky commented Apr 29, 2020

Uh oh!

davidwrighton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janvorli commented Apr 29, 2020

Uh oh!

janvorli left a comment

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky commented May 1, 2020

Uh oh!

AntonLapounov commented May 1, 2020

Uh oh!

AntonLapounov commented May 1, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AntonLapounov commented May 1, 2020

Uh oh!

CarolEidt commented May 2, 2020

Uh oh!

CarolEidt left a comment

Choose a reason for hiding this comment

Uh oh!

AntonLapounov commented May 2, 2020

Uh oh!

janvorli commented May 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

AntonLapounov commented Apr 28, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky commented Apr 29, 2020

Uh oh!

davidwrighton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janvorli commented Apr 29, 2020

Uh oh!

janvorli left a comment

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky commented May 1, 2020

Uh oh!

AntonLapounov commented May 1, 2020

Uh oh!

AntonLapounov commented Apr 28, 2020•
edited
Loading

AntonLapounov commented May 1, 2020•
edited
Loading

AntonLapounov commented Apr 28, 2020•
edited
Loading

AntonLapounov commented May 1, 2020•
edited
Loading

AntonLapounov commented Apr 28, 2020•
edited
Loading