Include dependently promoted fields in SSA #77238

SingleAccretion · 2022-10-19T23:03:35Z

Allowing for their participation in the SSA-based optimizations.

This changes two significant IR invariants:

It is not longer a correctness issue for morph to not mark a dependently promoted struct DNER / multi-reg - we remove the one hard requirement for this determination from the front-end. The soft requirement of CQ and making ref counting happy stays.
Uses of promoted locals can now implicitly refer to SSA variables. They are not renamed. The canonical example of such promoted locals are those created for multi-reg returns.

This change also makes lvIsMultiRegRet mostly unnecessary, though I plan to scale back its usage in a separate change.

2nd Round Diffs

ghost · 2022-10-19T23:03:50Z

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

Issue Details

Allowing for their participation in the SSA-based optimizations.

This changes two significant IR invariant:

It is not longer a correctness issue for morph to not mark a dependently promoted struct DNER / multi-reg - we remove the one hard requirement for this determination from the front-end. The soft requirement of CQ and making ref counting happy stays.
Uses of promoted locals can now implicitly refer to SSA variables. They are not renamed. The canonical example of such promoted locals are those created for multi-reg returns.

This change also makes lvIsMultiRegRet mostly unnecessary, though I plan to scale back its usage in a separate change.

Author:	SingleAccretion
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `community-contribution`
Milestone:	-

1) The definition of SIMPLE_NUM_COUNT was wrong. 2) SsaNumInfo::Composite, in the compact case, did not clear the old value. 3) SsaNumInfo::Composite, in the outlined case, did not copy the already (compactly) encoded simple names.

The load path needs to use the offset relative to the store's target location.

SingleAccretion · 2022-10-21T16:39:00Z

@dotnet/jit-contrib

This will need to be stressed and fuzzed.

EgorBo · 2022-10-21T17:02:42Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn

azure-pipelines · 2022-10-21T17:03:12Z

Azure Pipelines successfully started running 3 pipeline(s).

SingleAccretion · 2022-10-22T17:19:31Z

The CI results are a bit hard to interpret for known-ness right now (e. g. libraries stress has a little under 900 failures), Fuzzlyn looked clean-ish at least.

BruceForstall · 2022-10-26T05:09:25Z

Stress should be cleaner now.

BruceForstall · 2022-10-26T05:09:30Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn

azure-pipelines · 2022-10-26T05:10:04Z

Azure Pipelines successfully started running 3 pipeline(s).

SingleAccretion · 2022-10-26T17:51:07Z

Some (aka a lot of) stress (and Fuzzlyn) failures with an assert:

    JIT\Directed\Convert\value_numbering_checked_casts_of_constants\value_numbering_checked_casts_of_constants.cmd [FAIL]
      
      Assert failure(PID 4612 [0x00001204], Thread: 3588 [0x0e04]): Assertion failed 'isCandidateVar(fieldVarDsc) == isMultiReg' in 'ValueNumberingCheckedCastsOfConstants:<TestCastingSingleToInt64>g__ConfirmSingleOneFullDecrementUnderByteMaxValueCastToInt64IsFoldedCorrectly|10_27()' during 'Linear scan register alloc' (IL size 92; hash 0xfaebe139; Tier0-MinOpts)
      
          File: D:\a\_work\1\s\src\coreclr\jit\lsrabuild.cpp Line: 1269
          Image: C:\h\w\BB5E09CF\p\corerun.exe

Edit: this is fallout from #76263. Fix incoming...

SingleAccretion · 2022-10-26T19:28:00Z

Apart from the above, failures looked known.

AndyAyersMS · 2022-10-27T17:08:12Z

Hmm, odd that it got closed -- not sure how this happened.

SingleAccretion · 2022-10-27T17:12:15Z

Presumably the same curious GH behavior we've seen once, with this line

should fix stress failures

in #77502 to blame...

AndyAyersMS · 2022-11-04T15:33:57Z

Going to bounce this so it runs with ssa checking.

src/coreclr/jit/valuenum.cpp

AndyAyersMS · 2022-11-04T15:45:36Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn

azure-pipelines · 2022-11-04T15:46:15Z

Azure Pipelines successfully started running 3 pipeline(s).

AndyAyersMS · 2022-11-04T18:15:14Z

libraries jitstress has some known failures; coreclr jitstress was running clean recently.

SingleAccretion · 2022-11-04T18:57:53Z

Starting to collect failures.

Stress
- PAL memcpy tests: not related; opened c_runtime.memcpy PAL tests are failing #77914.
- GC/Regressions/Github/Runtime_76219: does look related, I remember seeing it fail the same way some time ago, and we have an identical, relatively recent, failure.
Libraries stress:
- GC/LargeMemory/Regressions/largearraytest: Test failure GC/LargeMemory/Regressions/largearraytest/largearraytest.sh #77477.
- Test failure System.Transactions.Tests.OleTxTests.ImplicitDistributedTransactions_cannot_be_changed_after_being_read_as_true #77241
- Microsoft.VisualBasic.Core.Tests: https://dev.azure.com/dnceng-public/public/_build/results?buildId=73254&view=ms.vss-test-web.build-test-results-tab&runId=1512312&resultId=197563&paneView=dotnet-dnceng.dnceng-anon-build-release-tasks.helix-anon-test-information-tab - Assert failure: !PreemptiveGCDisabled() #70307:

 # ChildEBP RetAddr      
00 02fe9c48 724aa24d     coreclr!FailFastOnAssert+0x21 [D:\a\_work\1\s\src\coreclr\utilcode\debug.cpp @ 60] 
01 02fe9c48 724a9967     coreclr!_DbgBreakCheck+0x329 [D:\a\_work\1\s\src\coreclr\utilcode\debug.cpp @ 247] 
02 02fe9cb8 724a9c91     coreclr!_DbgBreakCheckNoThrow+0x51 [D:\a\_work\1\s\src\coreclr\utilcode\debug.cpp @ 307] 
03 02fe9d54 72147fe8     coreclr!DbgAssertDialog+0x20c [D:\a\_work\1\s\src\coreclr\utilcode\debug.cpp @ 459] 
04 02fe9dd4 725aff92     coreclr!Thread::DetachThread+0xcf [D:\a\_work\1\s\src\coreclr\vm\threads.cpp @ 981] 
05 02fe9e40 725c2396     coreclr!TlsDestructionMonitor::~TlsDestructionMonitor+0x8f [D:\a\_work\1\s\src\coreclr\vm\ceemain.cpp @ 1764] 
06 02fe9e58 7727ea4e     coreclr!__dyn_tls_dtor+0x86 [D:\a\_work\1\s\src\vctools\crt\vcstartup\src\tls\tlsdtor.cpp @ 123] 
WARNING: Stack unwind information not available. Following frames may be wrong.
07 02fe9e78 7724eeb6     ntdll!RtlDecompressBuffer+0xde
08 02fe9ec8 772a3c6a     ntdll!LdrShutdownThread+0x386
09 02fe9f10 77262d7f     ntdll!LdrSetAppCompatDllRedirectionCallback+0x1052a
0a 02fe9fa8 77263886     ntdll!LdrShutdownProcess+0x15f
0b 02fea07c 7689b3e3     ntdll!RtlExitUserProcess+0x96
0c 02fea090 00000000     kernel32!ExitProcess+0x13

Fuzzlyn:
- JIT: Assertion failed '!"Write to unaliased local overlaps outstanding read"' during 'Rationalize IR' #77773.

SingleAccretion · 2022-11-04T20:43:29Z

The failures have been accounted for above, I believe all are known.

kunalspathak · 2022-11-07T15:55:28Z

Some (aka a lot of) stress (and Fuzzlyn) failures with an assert:

    JIT\Directed\Convert\value_numbering_checked_casts_of_constants\value_numbering_checked_casts_of_constants.cmd [FAIL]
      
      Assert failure(PID 4612 [0x00001204], Thread: 3588 [0x0e04]): Assertion failed 'isCandidateVar(fieldVarDsc) == isMultiReg' in 'ValueNumberingCheckedCastsOfConstants:<TestCastingSingleToInt64>g__ConfirmSingleOneFullDecrementUnderByteMaxValueCastToInt64IsFoldedCorrectly|10_27()' during 'Linear scan register alloc' (IL size 92; hash 0xfaebe139; Tier0-MinOpts)
      
          File: D:\a\_work\1\s\src\coreclr\jit\lsrabuild.cpp Line: 1269
          Image: C:\h\w\BB5E09CF\p\corerun.exe

Edit: this is fallout from #76263. Fix incoming...

This was also hit in Antigen in yesterday's run. Was this ever fixed?
Edit: It was on linux/arm

SingleAccretion · 2022-11-07T16:04:34Z

Was this ever fixed?

Yes, it was fixed in #77502. Evidently not fully...

EgorBo · 2022-11-10T17:46:27Z

Improvements on arm64-ubuntu: dotnet/perf-autofiling-issues#9679

Remove quirks

1498da4

ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 19, 2022

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 19, 2022

build-analysis bot mentioned this pull request Oct 20, 2022

Tracking issue for CI build timeouts #76454

Closed

SingleAccretion mentioned this pull request Oct 20, 2022

Add IR support for modeling multiple SSA definitions as one store #76139

Merged

SingleAccretion force-pushed the Dependent-Fields-In-SSA-Upstream branch 2 times, most recently from 3a19a20 to 295c147 Compare October 20, 2022 18:32

SingleAccretion added 3 commits October 20, 2022 21:39

Early prop fix for call asgs

dba93b8

Include all tracked locals in SSA

0273062

Remove a (now incorrect) comment

f6da0ad

SingleAccretion force-pushed the Dependent-Fields-In-SSA-Upstream branch from 295c147 to 497d70a Compare October 20, 2022 18:39

SingleAccretion added 3 commits October 21, 2022 17:17

Fix a typo

af6327c

Encoding fixes

99ed47c

1) The definition of SIMPLE_NUM_COUNT was wrong. 2) SsaNumInfo::Composite, in the compact case, did not clear the old value. 3) SsaNumInfo::Composite, in the outlined case, did not copy the already (compactly) encoded simple names.

Fix store numbering

44238c8

The load path needs to use the offset relative to the store's target location.

SingleAccretion force-pushed the Dependent-Fields-In-SSA-Upstream branch from 497d70a to 44238c8 Compare October 21, 2022 14:17

SingleAccretion marked this pull request as ready for review October 21, 2022 16:38

SingleAccretion mentioned this pull request Oct 23, 2022

Remove GT_ADDR nodes #11057

Closed

SingleAccretion mentioned this pull request Oct 26, 2022

Update lvaEnregMultiRegVars and lvaEnregEHVars when switching to MinOpts #77502

Merged

AndyAyersMS closed this in #77502 Oct 27, 2022

SingleAccretion reopened this Oct 27, 2022

AndyAyersMS closed this Nov 4, 2022

AndyAyersMS reopened this Nov 4, 2022

AndyAyersMS reviewed Nov 4, 2022

View reviewed changes

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

Clarify 'fieldValueOffset' calculations

dd88620

SingleAccretion mentioned this pull request Nov 5, 2022

DNER multi-reg locals early if they cannot be enregistered #77943

Closed

AndyAyersMS approved these changes Nov 6, 2022

View reviewed changes

AndyAyersMS merged commit 6bae351 into dotnet:main Nov 6, 2022

SingleAccretion deleted the Dependent-Fields-In-SSA-Upstream branch November 6, 2022 17:45

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include dependently promoted fields in SSA #77238

Include dependently promoted fields in SSA #77238

SingleAccretion commented Oct 19, 2022 •

edited

Loading

ghost commented Oct 19, 2022

SingleAccretion commented Oct 21, 2022

EgorBo commented Oct 21, 2022

azure-pipelines bot commented Oct 21, 2022

SingleAccretion commented Oct 22, 2022 •

edited

Loading

BruceForstall commented Oct 26, 2022

BruceForstall commented Oct 26, 2022

azure-pipelines bot commented Oct 26, 2022

SingleAccretion commented Oct 26, 2022 •

edited

Loading

SingleAccretion commented Oct 26, 2022

AndyAyersMS commented Oct 27, 2022

SingleAccretion commented Oct 27, 2022

AndyAyersMS commented Nov 4, 2022

AndyAyersMS commented Nov 4, 2022

azure-pipelines bot commented Nov 4, 2022

AndyAyersMS commented Nov 4, 2022

SingleAccretion commented Nov 4, 2022 •

edited

Loading

SingleAccretion commented Nov 4, 2022

kunalspathak commented Nov 7, 2022 •

edited

Loading

SingleAccretion commented Nov 7, 2022 •

edited

Loading

EgorBo commented Nov 10, 2022

Include dependently promoted fields in SSA #77238

Include dependently promoted fields in SSA #77238

Conversation

SingleAccretion commented Oct 19, 2022 • edited Loading

ghost commented Oct 19, 2022

SingleAccretion commented Oct 21, 2022

EgorBo commented Oct 21, 2022

azure-pipelines bot commented Oct 21, 2022

SingleAccretion commented Oct 22, 2022 • edited Loading

BruceForstall commented Oct 26, 2022

BruceForstall commented Oct 26, 2022

azure-pipelines bot commented Oct 26, 2022

SingleAccretion commented Oct 26, 2022 • edited Loading

SingleAccretion commented Oct 26, 2022

AndyAyersMS commented Oct 27, 2022

SingleAccretion commented Oct 27, 2022

AndyAyersMS commented Nov 4, 2022

AndyAyersMS commented Nov 4, 2022

azure-pipelines bot commented Nov 4, 2022

AndyAyersMS commented Nov 4, 2022

SingleAccretion commented Nov 4, 2022 • edited Loading

SingleAccretion commented Nov 4, 2022

kunalspathak commented Nov 7, 2022 • edited Loading

SingleAccretion commented Nov 7, 2022 • edited Loading

EgorBo commented Nov 10, 2022

SingleAccretion commented Oct 19, 2022 •

edited

Loading

SingleAccretion commented Oct 22, 2022 •

edited

Loading

SingleAccretion commented Oct 26, 2022 •

edited

Loading

SingleAccretion commented Nov 4, 2022 •

edited

Loading

kunalspathak commented Nov 7, 2022 •

edited

Loading

SingleAccretion commented Nov 7, 2022 •

edited

Loading