Move GetMethodTable and IsReference to JIT #88860

MichalPetryka · 2023-07-13T19:13:27Z

Moves the simple and most important RuntimeHelpers intrinsics to the JIT.

Extracted from #88543.

ghost · 2023-07-13T19:13:41Z

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

Issue Details

Moves the simple and most important RuntimeHelpers intrinsics to the JIT.

Extracted from #88543.

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

MichalPetryka · 2023-07-13T19:53:28Z

@MihuBot

src/coreclr/jit/assertionprop.cpp

src/coreclr/jit/importercalls.cpp

AndyAyersMS · 2023-07-13T23:18:02Z

src/coreclr/jit/assertionprop.cpp

@@ -4699,6 +4699,17 @@ GenTree* Compiler::optAssertionProp_Update(GenTree* newTree, GenTree* tree, Stat
            if (parent != nullptr)
            {
                parent->ReplaceOperand(useEdge, newTree);
+                if ((parent->gtOper == GT_IND) && newTree->IsIconHandle())


What has changed that requires modifying assertion prop?

VN can fold the method table indir to a constant handle which this place then replaces indirs on such method table with. This causes an assert in fgdiagnostic to fire complaining about the indir missing the invariant and nonfaulting flags.

But why couldn't this also happen without your changes? Is VN able to recognize something in your expansions but not the inlined IL intrinsics?

Yes, so basically we had IND(IND(knownObj)) and the inner IND(knonwObj) was folded by assertprop (with help of VN) to a CLASS_HANDLE so now we have IND(CLASS_HANDLE) but without proper flags (it's not really a correctness issue, more like annoying requirement from the fgdiagnostic)

Is VN able to recognize something in your expansions but not the inlined IL intrinsics?

I'd assume it's not able to recognize the field address + subtract magic there.

But why couldn't this also happen without your changes?

I think this should reproduce with any indir over the method table returned from gtNewMethodTableLookup, seems like nothing does that today though.

jkotas · 2023-07-14T09:17:53Z

crossgen crashing during the build with:

  /__w/1/s/src/coreclr/jit/fgdiagnostic.cpp:3290
  Assertion failed '!"Missing flags on tree"' in 'Interop+Sys:.cctor()' during 'Insert GC Polls' (IL size 27; hash 0xb6a186f0; FullOpts)

SingleAccretion · 2023-07-14T12:03:21Z

src/coreclr/jit/assertionprop.cpp

                parent->ReplaceOperand(useEdge, newTree);
+                parent->gtFlags |= gtGetFlagsForOperand(parent->gtOper, newTree);


This flag updating call should be moved to this place in morph:

runtime/src/coreclr/jit/morph.cpp

Lines 9568 to 9571 in 7980417

if (op1->IsIconHandle(GTF_ICON_OBJ_HDL))

{

tree->gtFlags |= (GTF_IND_INVARIANT | GTF_IND_NONFAULTING | GTF_IND_NONNULL);

}

SingleAccretion · 2023-07-14T12:04:45Z

src/coreclr/jit/gentree.cpp

+    GenTreeFlags flags = GTF_EMPTY;
+    if ((oper == GT_IND) && op->IsIconHandle())
+    {
+        flags |= GTF_IND_NONFAULTING;
+
+        GenTreeFlags handleKind = op->GetIconHandleFlag();
+        if ((handleKind != GTF_ICON_STATIC_HDL) && (handleKind != GTF_ICON_BBC_PTR) &&
+            (handleKind != GTF_ICON_GLOBAL_PTR))
+        {
+            flags |= GTF_IND_INVARIANT;
+        }
+    }


This invariantness setting is fragile; it does not check what type is being loaded.

Yes, I had the same concern - like why do we expect invariantess if we acess a byte location with long type

SingleAccretion · 2023-07-14T12:05:11Z

src/coreclr/jit/gentree.cpp

@@ -8214,6 +8240,7 @@ GenTreeBlk* Compiler::gtNewBlkIndir(ClassLayout* layout, GenTree* addr, GenTreeF
 //
 GenTreeIndir* Compiler::gtNewIndir(var_types typ, GenTree* addr, GenTreeFlags indirFlags)
 {
+    indirFlags |= gtGetFlagsForOperand(GT_IND, addr);


Why is this needed here? The caller is expected to pass the right indirFlags.

I bet it's not needed it's just how @MichalPetryka was trying to find the case where it's not set (turned out be assertprop)

JulieLeeMSFT · 2023-08-07T16:25:55Z

Hi @MichalPetryka, this is a kindly reminder. The .NET 8 RC1 snap is one week away.

JulieLeeMSFT · 2023-09-05T16:33:32Z

@MichalPetryka, I am moving this to .NET 9 because it is too late for .NET 8.

BruceForstall · 2023-09-13T05:05:22Z

@MichalPetryka Can you mark this "Draft" until it is ready for review again?

ghost · 2023-09-29T21:01:24Z

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

ghost · 2023-10-14T00:01:19Z

This pull request will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the pull request, but please note that it will be locked if it remains inactive for another 30 days.

Move GetMethodTable and IsReference to JIT

6749f5e

MichalPetryka requested review from marek-safar and MichalStrehovsky as code owners July 13, 2023 19:13

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

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 13, 2023

MichalPetryka mentioned this pull request Jul 13, 2023

Convert RuntimeHelpers intrinsics to JIT ones #88543

Closed

Format code

a1fe453

MihuBot mentioned this pull request Jul 13, 2023

[MichalPetryka] [X64] Move GetMethodTable and IsReference to JIT MihuBot/runtime-utils#127

Open

EgorBo reviewed Jul 13, 2023

View reviewed changes

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

EgorBo reviewed Jul 13, 2023

View reviewed changes

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

EgorBo reviewed Jul 13, 2023

View reviewed changes

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

EgorBo reviewed Jul 13, 2023

View reviewed changes

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

AndyAyersMS reviewed Jul 13, 2023

View reviewed changes

MichalPetryka added 2 commits July 14, 2023 01:36

Cleanup

34b03a2

Update comment

7e25581

SingleAccretion reviewed Jul 14, 2023

View reviewed changes

JulieLeeMSFT assigned EgorBo and MichalPetryka Jul 17, 2023

EgorBo added the needs-author-action An issue or pull request that requires more info or actions from the author. label Sep 4, 2023

JulieLeeMSFT added this to the 9.0.0 milestone Sep 5, 2023

MichalPetryka mentioned this pull request Sep 6, 2023

Assertion failed '!"Missing flags on tree"' #91505

Closed

MichalPetryka marked this pull request as draft September 15, 2023 16:06

ghost added the no-recent-activity label Sep 29, 2023

ghost closed this Oct 14, 2023

MichalPetryka mentioned this pull request Nov 3, 2023

JIT incorrectly adds handles to integer constants #94332

Closed

ghost locked as resolved and limited conversation to collaborators Nov 13, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move GetMethodTable and IsReference to JIT #88860

Move GetMethodTable and IsReference to JIT #88860

MichalPetryka commented Jul 13, 2023

ghost commented Jul 13, 2023

MichalPetryka commented Jul 13, 2023

AndyAyersMS Jul 13, 2023

MichalPetryka Jul 13, 2023

AndyAyersMS Jul 13, 2023

EgorBo Jul 13, 2023

MichalPetryka Jul 13, 2023

jkotas commented Jul 14, 2023

SingleAccretion Jul 14, 2023

SingleAccretion Jul 14, 2023

EgorBo Jul 14, 2023

SingleAccretion Jul 14, 2023

EgorBo Jul 14, 2023 •

edited

Loading

JulieLeeMSFT commented Aug 7, 2023

JulieLeeMSFT commented Sep 5, 2023

BruceForstall commented Sep 13, 2023

ghost commented Sep 29, 2023

ghost commented Oct 14, 2023

		parent->ReplaceOperand(useEdge, newTree);
		parent->gtFlags \|= gtGetFlagsForOperand(parent->gtOper, newTree);

	if (op1->IsIconHandle(GTF_ICON_OBJ_HDL))
	{
	tree->gtFlags \|= (GTF_IND_INVARIANT \| GTF_IND_NONFAULTING \| GTF_IND_NONNULL);
	}

Move GetMethodTable and IsReference to JIT #88860

Move GetMethodTable and IsReference to JIT #88860

Conversation

MichalPetryka commented Jul 13, 2023

ghost commented Jul 13, 2023

MichalPetryka commented Jul 13, 2023

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

jkotas commented Jul 14, 2023

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

EgorBo Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

JulieLeeMSFT commented Aug 7, 2023

JulieLeeMSFT commented Sep 5, 2023

BruceForstall commented Sep 13, 2023

ghost commented Sep 29, 2023

ghost commented Oct 14, 2023

EgorBo Jul 14, 2023 •

edited

Loading