JIT: Fix assertion due to remorph #110516

hez2010 · 2024-12-09T06:13:42Z

Extracted from #104906

We always set GTF_DEBUG_NODE_MORPHED in gtFoldExprCompare, gtFoldExprSpecial, gtFoldExprConditional and gtFoldExprHWIntrinsic if the tree is morphed, but not in gtFoldExprConst as it may creates a new tree that can be morphed again.
Especially, gtFoldExprSpecial can create a new COMMA containing a subtree of side effects, which leads to the assertion later when we try to remorph it, as newTree != oldTree can yield false positives for this. The subtree of COMMA is already morphed anyway so in this case we can avoid morphing it again by using a flag instead of relying on newTree != oldTree.

Fixes #107542

cc: @AndyAyersMS

hez2010 · 2024-12-09T09:20:57Z

No diffs

jakobbotsch · 2024-12-09T15:55:01Z

gtFoldExprConst is already conditioned on fgGlobalMorph when it folds overflowing arithmetic into a helper call. Can we just invoke morph from there in that case? It's not elegant, but it seems more elegant than this fix.

hez2010 · 2024-12-09T17:16:34Z

The issue here is that gtFoldExprSpecial can create a new COMMA (returning from gtWrapWithSideEffects) while its subtree is already morphed, so that the tree returned by gtFoldExpr is not equal to neither op1 nor op2. Then tree->AsOp()->gtOp1 = fgMorphTree(tree->AsOp()->gtOp1); caused the tree to be morphed again which leads to the assertion.

Regarding to the fix, in global morph, do we really need to morph op1 again? I think it should already be morphed anyway.

hez2010 · 2024-12-09T17:46:21Z

Let's see how CI going

hez2010 · 2024-12-09T18:44:48Z

Okay it becomes Assertion failed '(tree->gtDebugFlags & GTF_DEBUG_NODE_MORPHED) && "ERROR: Did not morph this node!" now.

I think it should already be morphed anyway.

Seems that this is not true.

hez2010 · 2024-12-12T15:35:11Z

Can we just invoke morph from there in that case? It's not elegant, but it seems more elegant than this fix.

It seems that similar patterns can also be produced by gtFoldExprHWIntrinsic and it's really not obvious where should we put the invocation. I would like to leave it as is to keep the fix simple instead.

jakobbotsch · 2024-12-12T15:44:05Z

Can we just invoke morph from there in that case? It's not elegant, but it seems more elegant than this fix.

It seems that similar patterns can also be produced by gtFoldExprHWIntrinsic and it's really not obvious where should we put the invocation. I would like to leave it as is to keep the fix simple instead.

What problem did you hit? gtFoldExprConst introduces a helper call in some cases. Morphing of a new helper call is very much necessary. But I don't see gtFoldExprHWIntrinsic introducing helper calls, and the point where it is invoked by morph makes me think that it should be unnecessary to call morph on anything in there.

jakobbotsch · 2024-12-12T16:10:01Z

I opened #110658 as an example of what I meant. Does this fix the assertion for the case in the issue, and for your own case?

hez2010 · 2024-12-12T17:18:25Z

I opened #110658 as an example of what I meant. Does this fix the assertion for the case in the issue, and for your own case?

Thanks! I will check it and see whether it fixes the issue I hit or not.

We had indirections set up where we'd `RuntimeExport` a method under a symbolic name and then `RuntimeImport` it elsewhere (or call the export from JIT-generated code). Most of this was motivated by the old Redhawk layering where things like casting and interface dispatch were part of Runtime.Base and not part of CoreLib. Since we decided if we ever reintroduce a Runtime.Base, we wouldn't have casting in it, this indirection will no longer be needed. Motivated by dotnet#110267 (and also the reason why I'm only doing it for `RuntimeExport`s used from the JIT right now). Once that PR gets in, calling methods through `RuntimeExport`s would actually become a bigger deoptimization than it is now.

This syncs the gcc version in toolchian file with what's in the newest container build. Fixes dotnet#110517

* Fixing step becomes a go * Trying to avoid step that becomes a go * Adding comments and more protections. * fixing comment * Checking if removing this comments, CI failures are gone. * Adding part of the changes to understand the failures on CI * Update src/coreclr/debug/ee/controller.cpp Co-authored-by: mikelle-rogers <[email protected]> * Fixing wrong fix. --------- Co-authored-by: mikelle-rogers <[email protected]>

Fix dotnet#109722 Co-authored-by: loadingcn <[email protected]>

…henNotSet (dotnet#110070) Accept "localdomain" as a valid DomainName on Android.

…efined. (dotnet#110506) * Fix build break for DATAS Stress Mode * Removed stress heap dynamic

…tate (dotnet#110449) Some methods have a nil token - for example, special runtime methods like array functions. When we tried to look up their IL version state, we were throwing an exception. Methods like this will have no versioning state, so check for a nil token and skip the lookup.

Based on measurements on the ASP.NET perf infra (BasicWebApi and TodosApi), using the GuardCF libraries in non-guard context doesn't have measurable impact on throughput. The impact on size is negligible. We can just have it enabled unconditionally on the native runtime bits.

…10669) This improves diagnostic in our measurement infra

This adds an invariant that there always exists an "init BB" throughout the JIT's phases. The init BB has the following properties: - It is only executed once, so it does not have any predecessors - It is not inside a try region, hence it dominates all other blocks in the function There are no further requirements on the BB. The init BB does not have to be `BBJ_ALWAYS` (unlike the old "scratch BB" concept). This is mainly because it philosophically does not make sense to insert IR at the end of the init BB, since earlier phases can have inserted arbitrary IR in them.

We're not using it anymore. Fixes dotnet#109961

…et#110678)

…assificationDataType` (dotnet#110602) - Add the different method desc types to the data descriptor - We only need their size right now - Add tests for different method desc classifications - Mostly fill-in for things I found we didn't cover - `GetNativeCode_StableEntryPoint_NonVtableSlot` is the one that actually hits the updated code in this change

…tnet#110322) * Add GetGCStressCodeCopy to ICodeVersions and IRuntimeTypeSystem

If we need to spill ref type args for a GDV, try to annotate the spilled locals with type information.

…10700) Found this while running the diagnostics repo SOS tests with the cDAC enabled. General sequence for the repro was: ``` !sethostruntime -none !bpmd <firstLocation> !bpmd <secondLocation> g g !clrstack ``` Printed stack shows `<unknown>` for some method(s). Between the first and second breakpoints, more methods were jitted and the corresponding code heap list updated. When a new method in the stack for `<secondLocation>` had the same code heap list as any method from `<firstLocation>`, we'd end up with a stale end address for the heap list and determine that the method was invalid (outside the address range). The cdac was assuming that the target would be created every time the state changes, but that is not the case (for the repro above, `!sethostruntime -none` resulted in not re-creating the target). We need to handle `IXCLRDataProcess::Flush` calls to clear out any cached data. With this change, the SOS tests with the cDAC enabled run successfully with both a Release and Debug cdacreader (on a Release runtime).

AndyAyersMS · 2024-12-18T21:02:42Z

Likely obsoleted by #110787

hez2010 · 2024-12-18T23:34:25Z

Okay, I'm closing this PR.

Try fixing remorph issue

0cbf3a2

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 9, 2024

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Dec 9, 2024

This was referenced Dec 9, 2024

iOS tests failing with WORKLOAD TIMED OUT - Killing user command. #108103

Open

System.Formats.Nrbf.Tests timeouts #110285

Closed

linux-armel checked CoreCLR_NonPortable build failing in CI #110517

Closed

hez2010 force-pushed the issue-107542 branch from 65b0dcc to 6fa132a Compare December 9, 2024 17:20

hez2010 force-pushed the issue-107542 branch from 6fa132a to 0cbf3a2 Compare December 9, 2024 18:45

jakobbotsch mentioned this pull request Dec 12, 2024

JIT: Avoid remorphing general gtFoldExpr return #110658

Closed

stephentoub and others added 9 commits December 14, 2024 17:59

Delete unused static field from List (dotnet#110515)

8e63d8d

Fix linux-armel build (dotnet#110514)

9df306f

This syncs the gcc version in toolchian file with what's in the newest container build. Fixes dotnet#110517

[wasi] bump wasmtime to 27 (dotnet#110524)

d9f5ee4

fix profiling env var names in profiling.md (dotnet#109764)

df70878

Fix dotnet#109722 Co-authored-by: loadingcn <[email protected]>

Change assertion in IPGlobalProperties_DomainName_ReturnsEmptyStringW…

6ade56a

…henNotSet (dotnet#110070) Accept "localdomain" as a valid DomainName on Android.

Added a fix for the build failure when STRESS_DYNAMIC_HEAP_COUNT is d…

1c960dd

…efined. (dotnet#110506) * Fix build break for DATAS Stress Mode * Removed stress heap dynamic

MichalStrehovsky and others added 14 commits December 14, 2024 17:59

[browser] NativeAOT-LLVM support in browser-bench (dotnet#110611)

32acefa

JIT: Remove always-true fgCanRelocateEHRegions (dotnet#110612)

cb8d141

[wasm] Add bench output log, to the file and to the console (dotnet#1…

a4ca48f

…10669) This improves diagnostic in our measurement infra

Remove FabricBot from area-owners.md (dotnet#110525)

d7cc790

We're not using it anymore. Fixes dotnet#109961

JIT: Spill newarr into temp (dotnet#110518)

15e01d4

[cdac] Fix ISOSDacInterface13.TraverseLoaderHeap parameter type (dotn…

07e85b6

…et#110678)

[cDAC] Implement GCCover portion of SOSDacImpl::GetMethodDescData (do…

97f8570

…tnet#110322) * Add GetGCStressCodeCopy to ICodeVersions and IRuntimeTypeSystem

Remove unused Precode::IsCorrectMethodDesc (dotnet#110703)

f1b1f3d

JIT: capture class types when spilling a GDV arg (dotnet#110675)

1502947

If we need to spill ref type args for a GDV, try to annotate the spilled locals with type information.

Improve codegen for Vector512.ExtractMostSignificatBits (dotnet#110662)

f52248f

hez2010 requested review from jeffhandley, marek-safar, akoeplinger, maraf, ilonatommy, lewing, pavelsavara, BrzVlad, kotlarmilos, steveisok, vitek-karas, lateralusX and MichalStrehovsky as code owners December 14, 2024 08:59

hez2010 closed this Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Fix assertion due to remorph #110516

JIT: Fix assertion due to remorph #110516

hez2010 commented Dec 9, 2024 •

edited

Loading

hez2010 commented Dec 9, 2024

jakobbotsch commented Dec 9, 2024

hez2010 commented Dec 9, 2024 •

edited

Loading

hez2010 commented Dec 9, 2024

hez2010 commented Dec 9, 2024

hez2010 commented Dec 12, 2024

jakobbotsch commented Dec 12, 2024

jakobbotsch commented Dec 12, 2024

hez2010 commented Dec 12, 2024

AndyAyersMS commented Dec 18, 2024

hez2010 commented Dec 18, 2024

JIT: Fix assertion due to remorph #110516

JIT: Fix assertion due to remorph #110516

Conversation

hez2010 commented Dec 9, 2024 • edited Loading

hez2010 commented Dec 9, 2024

jakobbotsch commented Dec 9, 2024

hez2010 commented Dec 9, 2024 • edited Loading

hez2010 commented Dec 9, 2024

hez2010 commented Dec 9, 2024

hez2010 commented Dec 12, 2024

jakobbotsch commented Dec 12, 2024

jakobbotsch commented Dec 12, 2024

hez2010 commented Dec 12, 2024

AndyAyersMS commented Dec 18, 2024

hez2010 commented Dec 18, 2024

hez2010 commented Dec 9, 2024 •

edited

Loading

hez2010 commented Dec 9, 2024 •

edited

Loading