Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert some old style intrinsics to NamedIntrinsic #62271

Merged
merged 9 commits into from
Dec 9, 2021

Conversation

am11
Copy link
Member

@am11 am11 commented Dec 2, 2021

Converted:

  • CORINFO_INTRINSIC_RTH_GetValueInternal -> NI_System_RuntimeTypeHandle_GetValueInternal
  • CORINFO_INTRINSIC_Object_GetType -> NI_System_Object_GetType
  • CORINFO_INTRINSIC_StubHelpers_GetStubContext -> NI_System_StubHelpers_GetStubContext
  • CORINFO_INTRINSIC_StubHelpers_GetStubContextAddr -> NI_System_StubHelpers_GetStubContext
  • CORINFO_INTRINSIC_StubHelpers_NextCallReturnAddress -> NI_System_StubHelpers_GetStubContext

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 2, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 2, 2021
@ghost
Copy link

ghost commented Dec 2, 2021

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

Issue Details

Converted:

  • CORINFO_INTRINSIC_RTH_GetValueInternal -> NI_System_RuntimeTypeHandle_GetValueInternal
  • CORINFO_INTRINSIC_Object_GetType -> NI_System_Object_GetType
  • CORINFO_INTRINSIC_StubHelpers_GetStubContext -> NI_System_StubHelpers_GetStubContext
  • CORINFO_INTRINSIC_StubHelpers_GetStubContextAddr -> NI_System_StubHelpers_GetStubContext
  • CORINFO_INTRINSIC_StubHelpers_NextCallReturnAddress -> NI_System_StubHelpers_GetStubContext
Author: am11
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Dec 2, 2021

Thanks for doing this, I recall there was some problem with GetType one in crossgen2 but I don't remember the details - let's see what CI thinks about it first 🙂

src/coreclr/jit/importer.cpp Outdated Show resolved Hide resolved
@am11 am11 force-pushed the feature/jit/namedintrinsic branch 3 times, most recently from cde5ed2 to e1a81fa Compare December 3, 2021 03:25
@am11 am11 force-pushed the feature/jit/namedintrinsic branch from 44c2cb9 to 638836f Compare December 3, 2021 06:18
@am11 am11 marked this pull request as ready for review December 3, 2021 06:19
@am11
Copy link
Member Author

am11 commented Dec 3, 2021

cc @EgorBo, @jkotas, PTAL.

I have added a TODO in interpreter, since named intrinsics are currently accessible only within the JIT. We can add a separate lookup table for interpreter, once someone will start working on fixing the interpreter.

@EgorBo
Copy link
Member

EgorBo commented Dec 3, 2021

@am11 LGTM, but could you please make sure it doesn't produce any diffs? because I'm a bit worried about the GetType
Unfortunately you can't check it via superpmi and the only option is jit-utils https://github.com/dotnet/jitutils

  1. build two runtimes on the same commits (e.g. C:\prj\runtime-base and C:\prj\runtime-diff)
    runtimes should be built like this:
build.cmd Clr+Libs -c Release
build.cmd Clr -c Checked
cd src\tests
build.cmd Checked generatelayoutonly
build.cmd Release generatelayoutonly
cd ..\..
  1. apply your changes to runtime-diff.
  2. run
jit-diff diff --output C:\prj\jit-diffs -f --core_root C:\prj\runtime-base\artifacts\tests\coreclr\windows.x64.Release\Tests\Core_Root --base C:\prj\runtime-base\artifacts\bin\coreclr\windows.x64.Checked --crossgen C:\prj\runtime-base\artifacts\tests\coreclr\windows.x64.Release\Tests\Core_Root\crossgen.exe

jit-diff diff --output C:\prj\jit-diffs -f --core_root C:\prj\runtime-diff\artifacts\tests\coreclr\windows.x64.Release\Tests\Core_Root --base C:\prj\runtime-diff\artifacts\bin\coreclr\windows.x64.Checked --crossgen C:\prj\runtime-diff\artifacts\tests\coreclr\windows.x64.Release\Tests\Core_Root\crossgen.exe

jit-analyze -b C:\prj\jit-diffs\dasmdiffs0\base -d C:\prj\jit-diffs\dasmdiffs1\base -r -c 100

it's a lot of steps but unfortunately for this specific case where we want to test changes in vm instead of jit it's the only option

@am11
Copy link
Member Author

am11 commented Dec 3, 2021

it's a lot of steps but unfortunately for this specific case where we want to test changes in vm instead of jit it's the only option

Yup, I remember, not my first time though dotnet/jitutils#275 ;)

@am11
Copy link
Member Author

am11 commented Dec 4, 2021

No diffs; only textual diffs are found.

Found 276 files with textual diffs.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 36335206
Total bytes of diff: 36335206
Total bytes of delta: 0 (0.00 % of base)
Total relative delta: NaN
    relative diff is a regression.


0 total files with Code Size differences (0 improved, 0 regressed), 273 unchanged.

0 total methods with Code Size differences (0 improved, 0 regressed), 244797 unchanged.

@EgorBo, some paths are changed in latest tooling (e,g dasmdiffs0 and dasmdiffs1 are now dasmset_1 and dasmset_2; Core_Root/crossgen is Core_Root/crossgen2/crossgen2). I have written an on-demand GH workflow to make it easier for future https://github.com/am11/CrossRepoCITesting/blob/02e887a/.github/workflows/jit-diff.yml. e.g. for this run https://github.com/am11/CrossRepoCITesting/actions/runs/1538330009, it was dispatched with these parameters:
on-demain jitdiff in ci

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

@am11
Copy link
Member Author

am11 commented Dec 8, 2021

@EgorBo, if there are no further comments, can this be merged? Thanks!

src/coreclr/vm/dllimport.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/corelib.cpp Outdated Show resolved Hide resolved
src/coreclr/vm/dllimport.cpp Outdated Show resolved Hide resolved
am11 and others added 2 commits December 9, 2021 06:17
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@jkotas
Copy link
Member

jkotas commented Dec 9, 2021

I think the pStubArg also needs to be deleted at internal static extern IntPtr GetDelegateTarget(Delegate pThis, ref IntPtr pStubArg); in StubHelpers.cs

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

I will be happy to help you to work through the issues that prevent getting rid of the few remaining old-style intrinsics.

@am11
Copy link
Member Author

am11 commented Dec 10, 2021

@jkotas, thank you! I have pushed the w.i.p commit to the same branch main...am11:feature/jit/namedintrinsic. Currently, it only compiles the native code, but failing on runtime.

$ artifacts/bin/testhost/net7.0-Linux-Debug-x64/shared/Microsoft.NETCore.App/7.0.0/corerun \
     $(pwd)/../hwapp/bin/Debug/net6.0/hwapp.dll

Stack overflow.
Repeat 261416 times:
--------------------------------
   at System.Runtime.Intrinsics.X86.Sse2.get_IsSupported()
--------------------------------
   at System.SpanHelpers.IndexOf(Char ByRef, Char, Int32)
   at System.String.wcslen(Char*)
   at System.String.Ctor(Char*)
   at System.AppContext.Setup(Char**, Char**, Int32)

However, first on the main branch; I am still trying to figure out a C# code sample which will make the importer detect CORINFO_INTRINSIC_ARRAY_* class of instrinsics. This line

JITDUMP("Intrinsic %s Recognized\n", impGetIntrinsicName(intrinsicID));
prints the other intrinsics in debug build (with DOTNET_JitDump=* ./corerun ...), but I am yet to see any of the array ones. 🤔

array like intrinsics which are recognized on hello-world start startup:

# runtime main branch
$ DOTNET_JitDump=* artifacts/bin/testhost/net7.0-Linux-Debug-x64/shared/Microsoft.NETCore.App/7.0.0/corerun \
     $(pwd)/../hwapp/bin/Debug/net6.0/hwapp.dll | grep -i intrinsic.*recog | sort -u | grep -i array
     
Named Intrinsic System.Array.GetLowerBound: Recognized
Named Intrinsic System.Runtime.CompilerServices.RuntimeHelpers.InitializeArray: Recognized
Named Intrinsic System.Runtime.InteropServices.MemoryMarshal.GetArrayDataReference: Not recognized

and these are the only old style intrinsics recognized:

$ DOTNET_JitDump=* artifacts/bin/testhost/net7.0-Linux-Debug-x64/shared/Microsoft.NETCore.App/7.0.0/corerun \
    $(pwd)/../hwapp/bin/Debug/net6.0/hwapp.dll | grep ^Intrinsic.*Recog | sort -u

Intrinsic CORINFO_INTRINSIC_ByReference_Ctor Recognized
Intrinsic CORINFO_INTRINSIC_ByReference_Value Recognized

@jkotas
Copy link
Member

jkotas commented Dec 10, 2021

failing on runtime

You have some mix up between CORINFO_FLG_JIT_INTRINSIC and CORINFO_FLG_INTRINSIC flags. I think you may want to delete CORINFO_FLG_JIT_INTRINSIC and just use CORINFO_FLG_INTRINSIC everywhere. It should fix the crash as side-effect.

I am still trying to figure out a C# code sample which will make the importer detect CORINFO_INTRINSIC_ARRAY_* class of instrinsics

These intrinsics are generated for multi-dimensional arrays.

@am11
Copy link
Member Author

am11 commented Dec 10, 2021

Thanks! All three of them are detected with this program.

var x = new int[2,2];
x[0,0] = 2;
x[1,0] = 1;

unsafe
{
    fixed(int *y = &x[0,0])
    {
        Console.WriteLine("{0}, {1}, {2}, {3}", x.GetLowerBound(0), x.GetUpperBound(1), x[1,0], (IntPtr)y);
    }
}

(dotnet build -p:AllowUnsafeBlocks=true)

@am11
Copy link
Member Author

am11 commented Dec 10, 2021

I've pushed the array intrinsic implementation to the branch. Currently, corerun is getting a sigsegv.
clrstack: http://sprunge.us/cT7pzU (croerun built from main branch on the same system successfully executes this app).
(aside: interestingly export DOTNET_SYSTEM_GLOBALIZATION_INVARIANT=1 has no effect on this initialization path, it always seem to execute)

@am11
Copy link
Member Author

am11 commented Dec 10, 2021

Also, the last line just before the crash in jit dumps is related to stub helper:

****** DONE compiling ILStubClass:IL_STUB_PInvoke(long,long,int):int

@jkotas
Copy link
Member

jkotas commented Dec 10, 2021

I do not see anything obvious that can explain this crash by just looking at the code.

Could you please open draft PR with the change to make it easy to comment on it? There are some minor issue that I would like to comment on.

@am11
Copy link
Member Author

am11 commented Dec 10, 2021

Opened #62639.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants