Fix HFA/HVA classification #37499

CarolEidt · 2020-06-05T17:50:37Z

sandreenko

Jit/ToolBox changes look good, with one question about compFloatingPointUsed.
I like how clean it looks with DOUBLE\Vector64 separation.

src/coreclr/src/vm/fieldmarshaler.h

sandreenko · 2020-06-06T00:11:42Z

src/coreclr/src/jit/codegencommon.cpp

-            // For 8-byte vectors corType will be returned as CORINFO_TYPE_DOUBLE.
-            result = TYP_SIMD16;
-            // This type may not appear elsewhere, but it will occupy a floating point register.
-            compFloatingPointUsed = true;


is it safe to drop compFloatingPointUsed = true;?

No, that was a mistake

src/coreclr/src/jit/morph.cpp

src/coreclr/tests/src/JIT/Regression/JitBlue/Runtime_35144/Runtime_35144.cs

CarolEidt · 2020-06-08T15:02:40Z

@davidwrighton - the crossgen2 tests are failing in CI, but passing on my local system. Can you advise me what I might be missing?

CarolEidt · 2020-06-09T00:49:32Z

@dotnet/dnceng - is this a known issue? I see it on two of my PRs, and at least one other as well:

##[error].packages/microsoft.dotnet.arcade.sdk/5.0.0-beta.20280.1/tools/RepositoryValidation.proj(33,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) License file content '/Users/runner/runners/2.169.1/work/1/s/LICENSE.TXT' doesn't match the expected license '/Users/runner/runners/2.169.1/work/1/s/.packages/microsoft.dotnet.arcade.sdk/5.0.0-beta.20280.1/tools/Licenses/MIT.txt'.

premun · 2020-06-09T07:17:37Z

@CarolEidt looks like folks are aware and fixed it already here: #37626

Rebase should get you going. Meanwhile we are looking into improving the process so that this doesn't happen again.

Fix dotnet#35144

CarolEidt · 2020-06-09T16:09:04Z

@jkoritzinsky @davidwrighton @sandreenko PTAL
@dotnet/jit-contrib

sandreenko

Jit/ToolBox changes LGTM.

CarolEidt · 2020-06-10T16:26:16Z

ping @jkoritzinsky @davidwrighton
perhaps @jkotas could review?

tannergooding · 2020-06-10T16:29:26Z

src/coreclr/src/inc/corhdr.h

+    CORINFO_HFA_ELEM_FLOAT,
+    CORINFO_HFA_ELEM_DOUBLE,
+    CORINFO_HFA_ELEM_VECTOR64,
+    CORINFO_HFA_ELEM_VECTOR128,


There aren't any architectures that have HVA for Vector256?

Looks like __vectorcall for x64 will eventually fit this bill: https://godbolt.org/z/DMX7Y2, but not for ARM32/ARM64 (which don't support Vector256<T>) nor the default calling conventions for x86 or x64 on Unix/Windows.

System V does treat a simple wrapper over __m256 as if it was directly __m256 however, not sure if that is classified as an HFA/HVA by the JIT...

Currently the runtime (JIT + VM) handles the System V ABI completely separately. HFA/HVA is ARM32/ARM64 only.

tannergooding · 2020-06-10T16:40:44Z

The changes make sense to me. I imagine the same code paths would need to be updated when Half support is added to the vector types?

AntonLapounov · 2020-06-10T17:12:01Z

src/coreclr/src/inc/corhdr.h

+    CORINFO_HFA_ELEM_VECTOR64,
+    CORINFO_HFA_ELEM_VECTOR128,


Technically VECTOR64 and VECTOR128 are HVAs, not HFAs. For that reason I have renamed HFA to either HA or HomogenousAggregate everywhere in managed code. If we introduce and expose a new enumeration, I would prefer to name it correctly from the beginning. However, I realize there are lots of hfa usage in JIT code, so leaving it up to you.

CarolEidt · 2020-06-10T17:12:28Z

The changes make sense to me. I imagine the same code paths would need to be updated when Half support is added to the vector types?

I confess that I haven't looked at the ABI requirements for Half, but I'm not sure why they would require a change, as (at least for now) the classification for vectors is independent of the element type.

jkoritzinsky

VM interop code LGTM

davidwrighton

As far as the change to the product goes, it looks ok, although sufficiently complex that I don't have complete confidence that I can review it to complete correctness. So I've made some suggestions of making the test more complex in ways that will cause our various automated runs to find bugs more effectively.

davidwrighton · 2020-06-11T19:22:47Z

src/coreclr/tests/src/JIT/Regression/JitBlue/Runtime_35144/Runtime_35144.cs

+static class Runtime_35144
+{
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static void Foo<T>(T x) { }


Could you change this test to pass an object after the T parameter? I would like it for GC Stress to cover the case where we use the calling convention processing logic of the VM to parse all of this.

In particular make the function be written as:

static void Foo<T>(T x, object o) { if (o.GetType() != typeof(string)) throw new Exception(); if (((string)o) != "SomeString") throw new Exception(); }

and call it passing a string "SomeString" as the extra parameter.

Also, if you could mark the Foo method as AggressiveOptimization so that it isn't compiled by crossgen that will also help chase down the case where crossgen compiles this different from what the normal runtime does.

@davidwrighton - Thanks for the review, and for the suggestions for improving the test!

Closes dotnet#36206, closes dotnet#36418. The issue was fixed by dotnet#37499.

* Reenable GitHub_26491. Closes #13355 * Reenable crossgen2 tests failing with old retyping/ They were fixed both with and without retyping. Closes #37883. * Reenable HVA merge cases. Closes #37341, closes #37880. * Reenable GitHub_35821. Closes #36206, closes #36418. The issue was fixed by #37499. * Delete extra lines that are no longer needed. #37506 was fixed in #38241. * delete a throwing init.

Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 5, 2020

CarolEidt force-pushed the Fix35144 branch from 4313220 to 2fe9d6e Compare June 5, 2020 23:07

CarolEidt mentioned this pull request Jun 5, 2020

ARM64: Vector64<T> is converted to HVA TYP_DOUBLE #35976

Closed

sandreenko reviewed Jun 6, 2020

View reviewed changes

src/coreclr/tests/src/JIT/Regression/JitBlue/Runtime_35144/Runtime_35144.cs Outdated Show resolved Hide resolved

CarolEidt force-pushed the Fix35144 branch from 13fbb01 to adac778 Compare June 7, 2020 02:24

CarolEidt added 2 commits June 9, 2020 02:49

Fix HFA/HVA classification

61b07f0

Fix dotnet#35144

PR Feedback and fix crossgen2

84192e6

CarolEidt force-pushed the Fix35144 branch from 6af6d50 to 84192e6 Compare June 9, 2020 09:49

CarolEidt requested review from jkoritzinsky, davidwrighton and sandreenko June 9, 2020 16:09

sandreenko approved these changes Jun 9, 2020

View reviewed changes

tannergooding reviewed Jun 10, 2020

View reviewed changes

AntonLapounov reviewed Jun 10, 2020

View reviewed changes

sandreenko mentioned this pull request Jun 10, 2020

Half: An IEEE 754 compliant float16 type #37630

Merged

jkoritzinsky approved these changes Jun 11, 2020

View reviewed changes

davidwrighton requested changes Jun 11, 2020

View reviewed changes

Test improvements

37e784b

davidwrighton approved these changes Jun 12, 2020

View reviewed changes

CarolEidt merged commit f7458a8 into dotnet:master Jun 12, 2020

CarolEidt deleted the Fix35144 branch June 12, 2020 21:15

sandreenko pushed a commit to sandreenko/runtime that referenced this pull request Jul 7, 2020

Reenable GitHub_35821.

c31f691

Closes dotnet#36206, closes dotnet#36418. The issue was fixed by dotnet#37499.

sandreenko mentioned this pull request Jul 7, 2020

Reenable fixed tests after noRetyping and similar changes. #38855

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HFA/HVA classification #37499

Fix HFA/HVA classification #37499

CarolEidt commented Jun 5, 2020 •

edited by kunalspathak

Loading

sandreenko left a comment

sandreenko Jun 6, 2020

CarolEidt Jun 9, 2020

CarolEidt commented Jun 8, 2020

CarolEidt commented Jun 9, 2020

premun commented Jun 9, 2020 •

edited

Loading

CarolEidt commented Jun 9, 2020

sandreenko left a comment

CarolEidt commented Jun 10, 2020

tannergooding Jun 10, 2020

tannergooding Jun 10, 2020

CarolEidt Jun 10, 2020

tannergooding commented Jun 10, 2020

AntonLapounov Jun 10, 2020

CarolEidt commented Jun 10, 2020

jkoritzinsky left a comment

davidwrighton left a comment

davidwrighton Jun 11, 2020

davidwrighton Jun 11, 2020

davidwrighton Jun 11, 2020

CarolEidt Jun 12, 2020

Fix HFA/HVA classification #37499

Fix HFA/HVA classification #37499

Conversation

CarolEidt commented Jun 5, 2020 • edited by kunalspathak Loading

sandreenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarolEidt commented Jun 8, 2020

CarolEidt commented Jun 9, 2020

premun commented Jun 9, 2020 • edited Loading

CarolEidt commented Jun 9, 2020

sandreenko left a comment

Choose a reason for hiding this comment

CarolEidt commented Jun 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding commented Jun 10, 2020

Choose a reason for hiding this comment

CarolEidt commented Jun 10, 2020

jkoritzinsky left a comment

Choose a reason for hiding this comment

davidwrighton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarolEidt commented Jun 5, 2020 •

edited by kunalspathak

Loading

premun commented Jun 9, 2020 •

edited

Loading