Fix System.Decimal aligment on ARM32 with crossgen2 by pavel-orekhov · Pull Request #38390 · dotnet/runtime

pavel-orekhov · 2020-06-25T14:30:31Z

Fix passing Decimal to funcs. Fixes tests/.../decimal.cs and half of tests/.../10w5d.cs

dnfadmin · 2020-06-25T14:30:44Z

All CLA requirements met.

MichalStrehovsky · 2020-06-25T18:31:33Z

This should be done similar to how other special types handled:

Make a new layout algorithm class deriving from FieldLayoutAlgorithm that does layout for System.Decimal specifically
Add it to the if block here:

runtime/src/coreclr/src/tools/aot/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs

Lines 49 to 67 in 3705185

    
           if (type.IsObject) 
        
               return _systemObjectFieldLayoutAlgorithm; 
        
           else if (type == UniversalCanonType) 
        
               throw new NotImplementedException(); 
        
           else if (type.IsRuntimeDeterminedType) 
        
               throw new NotImplementedException(); 
        
           else if (VectorOfTFieldLayoutAlgorithm.IsVectorOfTType(type)) 
        
           { 
        
               return _vectorOfTFieldLayoutAlgorithm; 
        
           } 
        
           else if (VectorFieldLayoutAlgorithm.IsVectorType(type)) 
        
           { 
        
               return _vectorFieldLayoutAlgorithm; 
        
           } 
        
           else 
        
           { 
        
               Debug.Assert(_r2rFieldLayoutAlgorithm != null); 
        
               return _r2rFieldLayoutAlgorithm; 
        
           }

MichalStrehovsky · 2020-06-25T18:33:15Z

src/coreclr/src/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

The helpers in InteropTypes.cs are specifically for interop marshallers. We don't have a central location with a "is this type X?" functionality. We place the functionality locally where it's needed.

Many thanks to you!
Ok. Moving modules to another assembly is not very good. So, is it right you suggests to copy-paste InteropTypes::IsSystemDecimal and IsCoreNamedType to R2RCompilerContext.FieldLayoutAlgorithm() / _r2rFieldLayoutAlgorithm and compare strings like

diff --git a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs index bbe1274cae3..d3210f2c5c7 100644 --- a/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs +++ b/src/coreclr/src/tools/crossgen2/ILCompiler.ReadyToRun/Compiler/ReadyToRunCompilerContext.cs @@ -60,6 +60,10 @@ public override FieldLayoutAlgorithm GetLayoutAlgorithmForType(DefType type) { return _vectorFieldLayoutAlgorithm; } + else if (type.IsValueType && type.Context.Target.Architecture == TargetArchitecture.ARM && IsSystemDecimal(type.Context, type)) + { + return _decimalOnARM32FieldLayoutAlgorithm; + } else { Debug.Assert(_r2rFieldLayoutAlgorithm != null); return _r2rFieldLayoutAlgorithm; }

?

Or i can use InteropTypes.IsSystemDecimal() directly here?

May be you suggests to create the flag DefType.isSystemDecimal in order to compare bits, not strings? But I don't know where to set this flag, where the Decimal class loads. :( Do you know it? Can you tell me that?

MichalStrehovsky · 2020-06-25T18:33:30Z

cc @dotnet/crossgen-contrib

jkotas · 2020-06-26T00:56:09Z

src/coreclr/src/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs

I think it would be easiest to check for CoreLib types by name in a method called from here, and apply the layout quirks that way.

Similar to how MethodTableBuilder::CheckForSystemTypes is done. I expect we will need most quirks from this method eventually.

Looking at MethodTableBuilder::CheckForSystemTypes - will we need anything else besides the Decimal addition? We already treat Vector<T> and the other vectors the way I propose for Decimal as well. I don't particularly care about the mechanism, but I do care about consistency.

Pro: Checking type in ReadyToRunCompilerContext allows to don't move InteropTypes to ILC.TypeSystem.R2R and allows to use existing InteropTypes.IsDecimal

Pro: Checking type in ReadyToRunCompilerContext removes checking of platform via ReadyToRunCompilerContext constructor

Contra: Checking type in ReadyToRunCompilerContext requires to write many code to implement other methods of FieldLayoutAlgorithm

Pro: checking type in MetadataFieldLayoutAlgorithm.cs exists already and is almost one-liner

Contra: copy-paste of isDecimal required or moving of InteropTypes to ILC.TypeSystem.R2R

Pro: Moving is already done

Contra: More changes in a branch for static struct will be to mitigate second half of the 10w5d.cs test.

Contra: Colleagues say, more types with broken alignment on ARM32 may arise.

So, Where? Please tell me the consensus!

We also faced with similar issues in Nullable types. If there are already exceptions for vectors and other types in ReadyToRunCompilerContext I think it would be preferred place.

What is the problem with Nullable types? Nullable types should not have any layout quirks.

If a nullable long enum isn't handled correctly, that is a bug in the general purpose type layout logic, not something that should be special cased.

LongE & so seem are fixed by #32663 / #32664.
But I afraid it is only masking of bug:
When (before) jit/methodical/fp/exgen/10w5d.cs Func_* don't compiled by cg2 but delayed to jit while ctors is done by cg2. So some difference in placement of static structs was (IMHO) between cg2 & jit compilers was and leaded to missfunction.
Right now placement of statics and using of it is by cg2 and you can't see difference with jit's ideas.

So bug (10w5d, LongE and so) may resurrect when cg2 and jit will be used. For example in eval case.

For Decimal, I am wondering whether we should fix the Decimal implementation to make this special casing unncessary. I will take a look at what it would take today.

LongE & so seem are fixed by #32663 / #32664.

I have reverted this workaround a few days ago. Could you please check whether it is still a problem on current master?

#32663 / #32664.

I have reverted this workaround a few days ago. Could you please check whether it is still a problem on current master?

With 4e5c8c0 (today's (20200629) master) the jit/methodical/fp/exgen/10w5d.cs is ok.

How to force 'delayed compilation' back I don't know (eval does not exists, imho). Sorry.
If you mean mass testing clr tests with cg2 please look to our night test results:
results.tar.gz

#38603 should be a cleaner fix for the Decimal issue.

pavel-orekhov · 2020-06-26T13:23:56Z

cc @alpencolt

* Fix passing Decimal to funcs. It fixes the JIT/Methodical/MDArray/DataTypes/decimal.cs

pavel-orekhov · 2020-06-29T13:24:42Z

def2b07 is @jkotas's style rewriting ;)

System.Decimal fields did not match Win32 DECIMAL type for historic reasons. It required type loader to have a quirk to artifically inflate System.Decimal alignment to match the alignment of Win32 DECIMAL type to make interop work well. This change is fixing the System.Decimal fields to match Win32 DECIMAL type and removing the quirk from the type loader since it does not belong there. The downsides are: - Slightly lower code quality on 32-bit platforms. 32-bit platforms are not our high performance targets anymore. - Explicit implementation of ISerializable is required on System.Decimal for binary serialization compatibility. Fixes dotnet#38390

System.Decimal fields did not match Win32 DECIMAL type for historic reasons. It required type loader to have a quirk to artificially inflate System.Decimal alignment to match the alignment of Win32 DECIMAL type to make interop work well. This change is fixing the System.Decimal fields to match Win32 DECIMAL type and removing the quirk from the type loader since it does not belong there. The downsides are: - Slightly lower code quality on 32-bit platforms. 32-bit platforms are not our high performance targets anymore. - Explicit implementation of ISerializable is required on System.Decimal for binary serialization compatibility. Fixes #38390

jkotas · 2020-06-30T16:46:26Z

@pavel-orekhov I have fixed decimal implementation to not require this quirk. Give it a try

t-mustafin · 2020-07-06T14:19:07Z

I hit on different input argument r1 usage generated by crossgen2 and Tier-1 compilation on lambda-expression containing decimal variable:

var soldOutProducts = products.Where(p => p.UnitsInStock > 0 && p.UnitPrice > 60.00M);

from Linq test.
Code by crossgen2 loads input decimal from r1+20:

00001A  6948           ldr     r0, [r1+20]
00001C  9004           str     r0, [sp+0x10]
00001E  6988           ldr     r0, [r1+24]
000020  9005           str     r0, [sp+0x14]
000022  69C8           ldr     r0, [r1+28] 
000024  9006           str     r0, [sp+0x18]
000026  6A08           ldr     r0, [r1+32]
000028  9007           str     r0, [sp+0x1c]

meanwhile tier-1 compiled code loads input decimal from r1+24:

00001A  F101 0018      add     r0, r1, 24
00001E  6804           ldr     r4, [r0]
000020  6845           ldr     r5, [r0+4]
000022  3008           adds    r0, 8
000024  6801           ldr     r1, [r0]
000026  6840           ldr     r0, [r0+4]
000028  9108           str     r1, [sp+0x20]
00002A  9009           str     r0, [sp+0x24]

That incompatibility leads to test fail after tier compilation of that lambda-expression. Test with turned off tier compilation finish successfully.
Linq.cg2.dump.txt
Linq.cg2.S.txt
Linq.cg2-tier.dump.txt
Linq.cg2-tier.S.txt

jkotas · 2020-07-06T15:00:16Z

This bug should not be specific to Decimal. Are you able to reproduce it with a custom type with long field? E.g. struct MyStruct { int a; int b; long c; }.

Looks like getClassAlignmentRequirement in crossgen2 may be returning wrong value on arm.

t-mustafin · 2020-07-06T20:11:46Z

Yes, it is reproduced with MyStruct { int a; int b; long c; }. Lambda-expression:

var soldOutProducts = products.Where(p => p.MyStruct.a > 0x300 || p.MyStruct.b > 0x50000 || p.MyStruct.c > 0x6000000);

Offsets generated by crossgen2:

000006  6A48           ldr     r0, [r1+36]
000008  F5B0 7F40      cmp     r0, 768
00000C  DC14           bgt     SHORT G_M16079_IG06
                        ;; bbWeight=1    PerfScore 3.00
G_M16079_IG03:
00000E  6A88           ldr     r0, [r1+40]
000010  F5B0 2FA0      cmp     r0, 0x50000
000014  DC10           bgt     SHORT G_M16079_IG06
000016  F101 002C      add     r0, r1, 44
00001A  6803           ldr     r3, [r0]
00001C  6840           ldr     r0, [r0+4]

Offsets generated by Tier-1:

G_M16079_IG02:
000006  6A88           ldr     r0, [r1+40]
000008  F5B0 7F40      cmp     r0, 768
00000C  DC14           bgt     SHORT G_M16079_IG06
                        ;; bbWeight=1    PerfScore 3.00
G_M16079_IG03:
00000E  6AC8           ldr     r0, [r1+44]
000010  F5B0 2FA0      cmp     r0, 0x50000
000014  DC10           bgt     SHORT G_M16079_IG06
000016  F101 0030      add     r0, r1, 48
00001A  6803           ldr     r3, [r0]
00001C  6840           ldr     r0, [r0+4]

mangod9 · 2020-07-06T20:21:46Z

@t-mustafin since this particular issues is closed, could you please create a new issue for this ? Thx.

Dotnet-GitSync-Bot added the area-crossgen2-coreclr label Jun 25, 2020

pavel-orekhov mentioned this pull request Jun 25, 2020

Bail out on CORINFO_STATIC_FIELD_ADDRESS like Crossgen1 does #32664

Merged

MichalStrehovsky reviewed Jun 25, 2020

View reviewed changes

jkotas reviewed Jun 26, 2020

View reviewed changes

[crossgen2] Fix System.Decimal aligment on ARM32

def2b07

* Fix passing Decimal to funcs. It fixes the JIT/Methodical/MDArray/DataTypes/decimal.cs

jkotas mentioned this pull request Jun 30, 2020

Remove decimal alignment quirk from the type loader #38603

Merged

jkotas closed this in #38603 Jun 30, 2020

t-mustafin mentioned this pull request Jul 6, 2020

crossgen2 getClassAlignmentRequirement returns wrong value on arm #38822

Closed

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

Conversation

pavel-orekhov commented Jun 25, 2020

Uh oh!

dnfadmin commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichalStrehovsky commented Jun 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-orekhov Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichalStrehovsky commented Jun 25, 2020

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!

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!

pavel-orekhov Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-orekhov Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pavel-orekhov commented Jun 26, 2020

Uh oh!

pavel-orekhov commented Jun 29, 2020

Uh oh!

jkotas commented Jun 30, 2020

Uh oh!

t-mustafin commented Jul 6, 2020

Uh oh!

jkotas commented Jul 6, 2020

Uh oh!

t-mustafin commented Jul 6, 2020

Uh oh!

mangod9 commented Jul 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

dnfadmin commented Jun 25, 2020 •

edited

Loading

pavel-orekhov Jun 25, 2020 •

edited

Loading

pavel-orekhov Jun 29, 2020 •

edited

Loading

pavel-orekhov Jun 29, 2020 •

edited

Loading