Make JIT<->EE printing methods consistent and support nested classes when printing class names #76505

jakobbotsch · 2022-10-02T15:35:11Z

Switch appendClassName/getClassName, getFieldName and getMethodName into respectively printClassName, printFieldName and printMethodName that all are consistent with printObjectDescription in buffer handling, and all use UTF8
Change printClassName to support nested classes, fixing the handling of these in the JIT (in particular for method sets)
Factor handling in SPMI/ILC/crossgen2 for these methods now that they are consistent
Add a JitTypeNameFormatter.cs for crossgen2/ILC that does exactly what printClassName needs to be consistent with the VM
Remove concept of "native" CORINFO_METHOD_HANDLE (eeMarkNativeTarget and co.). This was unused before.
Remove CEEInfo::getHelperName. We were keeping a list of this in the JIT anyway under FEATURE_SIMD, which is defined practically everywhere.
Remove includeNamespaces in JIT printing that was always passed as true
Change the hackishX names on missing SPMI data into <unknown X> instead, e.g. <unknown method>

The change does complicate the JIT side slightly since the JIT owns all the buffers now. I have added a buffer argument to the various JIT helper methods (e.g. eeGetFieldName) that ensures we don't unnecessarily allocate memory in common short cases when printing these, so there are a few additional parameters passed, but in general I don't think it looks too bad. In a few recursive functions I have introduced a small lambda to avoid bloating the frame size by a lot (assuming it doesn't just get inlined).
I personally prefer the consistency and flexibility over the slight increase in complexity.

* Switch JIT to use appendClassName instead of getClassNameFromMetadata. This way it supports enclosing classes too when printing class names. * Switch appendClassName to UTF8 * Switch SIMD type recognition to use getTypeInstantiationArgument and getClassNameFromMetadata, to use of appendClassName (and depending on how it formats instantiations) * Add TypeString::FormatNoInst to avoid formatting instantiations, and use this for appendClassName

* Remove getClassName from JIT-EE interface * Remove TypeString.cs in favor of JitTypeNameFormatter that does exactly what we need * Bump JIT-EE GUID

ghost · 2022-10-02T15:35:27Z

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

Issue Details

Switch JIT to use appendClassName instead of getClassNameFromMetadata.
This way it supports enclosing classes too when printing class names.
Switch appendClassName to UTF8
Switch SIMD type recognition to use getTypeInstantiationArgument and
getClassNameFromMetadata, to avoid use of appendClassName (and depending on
how it formats instantiations)
Add TypeString::FormatNoInst to avoid formatting instantiations, and
use this for appendClassName
Remove getClassName, it is unused now
Remove TypeString.cs in crossgen2/ILC in favor of a new JitTypeNameFormatter.cs that does exactly what appendClassName needs

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

SingleAccretion · 2022-10-02T15:46:52Z

src/coreclr/jit/simd.cpp

-                {
-                    size = getSIMDVectorRegisterByteLength();
+                CORINFO_CLASS_HANDLE typeArgHnd = info.compCompHnd->getTypeInstantiationArgument(typeHnd, 0);
+                simdBaseJitType                 = info.compCompHnd->asCorInfoType(typeArgHnd);


asCorInfoType

Will the case of Vector<MyEnum> be rejected properly?

I probably need getTypeForPrimitiveNumericClass instead.

In fact, I can see that the code below for hw intrinsics is doing exactly this but with getTypeForPrimitiveNumericClass, so looks like that's definitely the way to go.

src/coreclr/tools/aot/ILCompiler.ReadyToRun/ObjectWriter/OutputInfoBuilder.cs

Apparently caller expects the size to be right even for unsupported SIMD types.

src/coreclr/tools/aot/ILCompiler.RyuJit/ILCompiler.RyuJit.csproj

src/coreclr/jit/ee_il_dll.cpp

jkotas · 2022-10-03T14:43:46Z

Test failing with:

Assert failure(PID 4100 [0x00001004], Thread: 14536 [0x38c8]): Assertion failed 'sizeBytes == 0' in 'JIT.HardwareIntrinsics.Arm.LoadPairScalarVector64_Int32:RunBasicScenario():this' during 'Importation' (IL size 78; hash 0x426d4f3e; FullOpts)

jakobbotsch · 2022-10-03T14:45:32Z

Yep, I'm looking into it. The other failure looks like #48798.

jakobbotsch · 2022-10-03T20:55:56Z

It's very odd that my PR has failed twice with the #48798 symptom now, and that issue has no other failures. I'll try to see if I can repro this tomorrow.

* Avoid multiple calls into EE in common case of short strings * Remove StringPrinter::AllocToPrint complexity in face of above * Optimize appendClassName on EE side with inlined implementation of what we need from TypeString that avoids multiple roundtrips in and out of UTF8. Also copy resulting string directly into final buffer. * Revert TypeString::FormatNoInst

…asses

jakobbotsch · 2022-11-14T16:59:50Z

src/coreclr/tools/superpmi/mcs/verbildump.cpp


    printf("// ProcessName - '%s'\n", mc->cr->repProcessName());
    printf(".assembly extern mscorlib{}\n");
-    printf(".assembly %s{}\n", moduleName);
+    printf(".assembly dumped_asm\n");


This "scope name" was not actually the module name.

jakobbotsch · 2022-11-14T17:02:07Z

src/coreclr/jit/ee_il_dll.cpp

+    vfprintf(jitstdout, fmt, vl);
+    va_end(vl);
+}
+#endif


This was part of a #else of some #ifdef I ended up removing, so I've moved it here.

jakobbotsch · 2022-11-14T17:04:52Z

src/coreclr/jit/eeinterface.cpp

+static const char* s_jitHelperNames[CORINFO_HELP_COUNT] = {
+#define JITHELPER(code, pfnHelper, sig) #code,
+#define DYNAMICJITHELPER(code, pfnHelper, sig) #code,
+#include "jithelpers.h"
+};


This was defined in ee_il_dll.cpp before. I've moved most of this printing into eeinterface.cpp.
It was also conditionally defined, but one of the conditions was FEATURE_SIMD which is included for most of our targets (I think arm32 is the big exception). It seems reasonable to me to have the table unconditionally and remove the JIT-EE function.

jakobbotsch · 2022-11-14T17:06:44Z

src/coreclr/jit/gentree.cpp

+            auto disp = [&]() {
+                char buffer[256];
+                printf(" %s", eeGetFieldName(tree->AsField()->gtFldHnd, true, buffer, sizeof(buffer)));
+            };
+            disp();


Just avoiding bloating the stack frame here, since gtDispTree is recursive (well, it is left up to the C++ compiler's inlining).

jakobbotsch · 2022-11-14T17:07:35Z

src/coreclr/jit/importer.cpp

-                                        assert(!strcmp(info.compCompHnd->getFieldName(hasValueFldHnd, nullptr),
-                                                       "hasValue"));


We could print this (or better yet, introduce getFieldNameFromMetadata), but I don't think it's worth the effort to have the assert.

jakobbotsch · 2022-11-14T17:08:46Z

cc @dotnet/jit-contrib for the JIT/SPMI parts, @jkotas for the other parts

I'm going to wait with merging this until one of the other JIT-EE changes in the pipeline go in this week.

src/coreclr/vm/jitinterface.cpp

jkotas

VM/AOT parts LGTM

BruceForstall · 2022-11-15T22:48:57Z

src/coreclr/inc/corinfo.h

@@ -2282,7 +2282,7 @@ class ICorStaticInfo
    //                          from that first attempt was not big enough.
    //
    // Return Value:
-    //    Bytes written to the given buffer, the range is [0..bufferSize)
+    //    Bytes written to the given buffer excluding the null terminator. The range is [0..bufferSize).


It would be useful to note in the comment above that bufferSize is in bytes, and *pRequiredBufferSize is in bytes (if pRequiredBufferSize is non-nullptr).

Can buffer be nullptr if bufferSize == 0? This would be the one case where buffer can't be null terminated, of course, and where the documented return value range [0..0) makes no sense.

Can *pRequiredBufferSize == 0? Since it needs to include space for a null terminator, I presume it needs to be at least 1.

Can *pRequiredBufferSize == 1? That is, only a null terminator, and no text?

If bufferSize > 0, and the return value is < bufferSize - 1, are we guaranteed that the entire string is written (i.e., there is no truncation)? Does this imply return value == *pRequiredBufferSize - 1? It would be useful to document these conditions.

What happens if handle is illegal? Crash? Null-terminated buffer (if bufferSize > 0)?

Presumably you can do:

size_t actualSize; size_t sz = printObjectDescription(h, nullptr, 0, &actualSize); assert(sz == 0); assert(actualSize > 0); char* buffer = new char[actualSize]; sz = printObjectDescription(h, buffer, actualSize, &actualSize); assert(sz == actualSize - 1);

Can you please take a look at the extra docs I've added in the latest commits?

src/coreclr/tools/superpmi/superpmi-shared/asmdumper.cpp

src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp

src/coreclr/jit/eeinterface.cpp

…ous PR

BruceForstall

LGTM

jakobbotsch added 3 commits October 2, 2022 16:42

Further cleanups/fixes

a176df0

* Remove getClassName from JIT-EE interface * Remove TypeString.cs in favor of JitTypeNameFormatter that does exactly what we need * Bump JIT-EE GUID

Run jit-format

3bfb46f

jakobbotsch requested a review from MichalStrehovsky as a code owner October 2, 2022 15:35

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 2, 2022

ghost assigned jakobbotsch Oct 2, 2022

SingleAccretion reviewed Oct 2, 2022

View reviewed changes

jakobbotsch commented Oct 2, 2022

View reviewed changes

src/coreclr/tools/aot/ILCompiler.ReadyToRun/ObjectWriter/OutputInfoBuilder.cs Outdated Show resolved Hide resolved

jakobbotsch added 4 commits October 2, 2022 18:01

Revert TypeString removal

e8fa31d

Use getTypeForPrimitiveNumericClass instead

42501aa

Nit

70d4370

Fix unsupported test

e7d0aa9

Apparently caller expects the size to be right even for unsupported SIMD types.

jakobbotsch changed the title ~~Clean up JIT<->EE "get class name" methods, support nested classes in JitDisasm~~ Clean up JIT<->EE "get class name" methods, support nested classes in method sets Oct 2, 2022

jkotas reviewed Oct 3, 2022

View reviewed changes

src/coreclr/tools/aot/ILCompiler.RyuJit/ILCompiler.RyuJit.csproj Outdated Show resolved Hide resolved

jkotas reviewed Oct 3, 2022

View reviewed changes

src/coreclr/jit/ee_il_dll.cpp Outdated Show resolved Hide resolved

jakobbotsch added 2 commits October 3, 2022 11:25

Address feedback

fcb4cc3

Remove leftover file

0449899

runfoapp bot mentioned this pull request Oct 3, 2022

Test_EventSource_EtwManifestGeneration* tests failing in CI #48798

Closed

Fix test failure

04def1f

jakobbotsch added 3 commits October 4, 2022 03:30

Remove StringPrinter::Grow

9b29a16

Fix contract, fix compiler error on x64

9bc6620

jakobbotsch mentioned this pull request Oct 4, 2022

SString::AppendUTF8 converts 'this' to UTF16 if it is non-ascii #76581

Open

jakobbotsch added 3 commits October 4, 2022 03:58

Fix escaping

ffaf9a4

Fix GCC build

0f37c4e

Clarify comment

77e799a

jakobbotsch added 10 commits November 13, 2022 19:57

Avoid unnecessary diff 2

a5b7986

Minor clean up for isEnum

2f93dcb

Avoid another diff

f8bac91

Remove a couple more hackishX in favor of <unknown X>

2f215cb

More consistent buffer size

79ed341

GC_NOTRIGGER

d88c235

Remove getHelperName

486a4fc

Few fixes

b45f7dd

Merge branch 'main' of github.com:dotnet/runtime into print-nested-cl…

6676673

…asses

Handle a few more cases in JitTypeNameFormatter

a8d8be1

jakobbotsch marked this pull request as ready for review November 14, 2022 16:50

jakobbotsch commented Nov 14, 2022

View reviewed changes

jkotas reviewed Nov 14, 2022

View reviewed changes

src/coreclr/vm/jitinterface.cpp Outdated Show resolved Hide resolved

jkotas approved these changes Nov 14, 2022

View reviewed changes

Update src/coreclr/vm/jitinterface.cpp

2b7e2ae

BruceForstall requested changes Nov 15, 2022

View reviewed changes

ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Nov 15, 2022

jakobbotsch added 3 commits November 16, 2022 11:47

Address feedback, remove outdated comment I forgot to remove in previ…

8c7a16c

…ous PR

Address some more feedback

65c06f7

Small fix

9aa933c

BruceForstall approved these changes Nov 17, 2022

View reviewed changes

jakobbotsch merged commit df00d19 into dotnet:main Nov 20, 2022

jakobbotsch deleted the print-nested-classes branch November 20, 2022 12:28

ghost locked as resolved and limited conversation to collaborators Dec 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make JIT<->EE printing methods consistent and support nested classes when printing class names #76505

Make JIT<->EE printing methods consistent and support nested classes when printing class names #76505

jakobbotsch commented Oct 2, 2022 •

edited

Loading

ghost commented Oct 2, 2022

SingleAccretion Oct 2, 2022

jakobbotsch Oct 2, 2022

jakobbotsch Oct 2, 2022

jkotas commented Oct 3, 2022

jakobbotsch commented Oct 3, 2022

jakobbotsch commented Oct 3, 2022

jakobbotsch Nov 14, 2022

jakobbotsch Nov 14, 2022

jakobbotsch Nov 14, 2022

jakobbotsch Nov 14, 2022

jakobbotsch Nov 14, 2022

jakobbotsch commented Nov 14, 2022

jkotas left a comment

BruceForstall Nov 15, 2022

jakobbotsch Nov 16, 2022

BruceForstall left a comment

		assert(!strcmp(info.compCompHnd->getFieldName(hasValueFldHnd, nullptr),
		"hasValue"));

Make JIT<->EE printing methods consistent and support nested classes when printing class names #76505

Make JIT<->EE printing methods consistent and support nested classes when printing class names #76505

Conversation

jakobbotsch commented Oct 2, 2022 • edited Loading

ghost commented Oct 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Oct 3, 2022

jakobbotsch commented Oct 3, 2022

jakobbotsch commented Oct 3, 2022

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

jakobbotsch commented Nov 14, 2022

jkotas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall left a comment

Choose a reason for hiding this comment

jakobbotsch commented Oct 2, 2022 •

edited

Loading