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.

Sign up for GitHub

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

Jump to bottom

JIT: libraries jitstress Assertion failed 'doesVNMatch' during 'Assertion prop' #109745

Closed
AndyAyersMS opened this issue Nov 12, 2024 · 5 comments · Fixed by #109959
Closed

JIT: libraries jitstress Assertion failed 'doesVNMatch' during 'Assertion prop' #109745

AndyAyersMS opened this issue Nov 12, 2024 · 5 comments · Fixed by #109959
Assignees
@jakobbotsch
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' in-pr There is an active PR which will close this issue when it is merged Known Build Error Use this to report build issues in the .NET Helix tab
Milestone
10.0.0

Comments

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Nov 12, 2024 •
edited by build-analysis bot
Loading

x86 libraries jitstress

https://dev.azure.com/dnceng-public/public/_build/results?buildId=867933&view=ms.vss-test-web.build-test-results-tab

"C:\h\w\A3450896\p\dotnet.exe" exec --runtimeconfig System.Collections.Immutable.Tests.runtimeconfig.json --depsfile System.Collections.Immutable.Tests.deps.json xunit.console.dll System.Collections.Immutable.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: System.Collections.Immutable.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Collections.Immutable.Tests (found 4292 of 4397 test cases)
  Starting:    System.Collections.Immutable.Tests (parallel test collections = on [4 threads], stop on fail = off)

DOTNET_JitStress=1
DOTNET_TieredCompilation=0

Assert failure(PID 4116 [0x00001014], Thread: 9356 [0x248c]): Assertion failed 'doesVNMatch' in 'System.Collections.Immutable.Tests.ImmutableArrayTest+<ChangeType>d__192`1[int]:MoveNext():ubyte:this' during 'Assertion prop' (IL size 362; hash 0x08aa2d36; FullOpts)

    File: D:\a\_work\1\s\src\coreclr\jit\assertionprop.cpp:1486
    Image: C:\h\w\A3450896\p\dotnet.exe

Known Issue Error Message

Fill the error message using step by step known issues guidance.

{
  "ErrorMessage": "Assertion failed 'doesVNMatch'",
  "ErrorPattern": "",
  "BuildRetry": false,
  "ExcludeConsoleLog": false
}

Known issue validation

Build: 🔎 https://dev.azure.com/dnceng-public/public/_build/results?buildId=867933
Error message validated: [Assertion failed 'doesVNMatch']
Result validation: ✅ Known issue matched with the provided build.
Validation performed at: 11/14/2024 12:58:45 PM UTC

Report

Build Definition Test Pull Request
875331 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #109987
873656 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #106557
873420 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #109826
873272 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #109957
872587 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #109808
871840 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #109719
871630 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #109861
871309 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #109804
871150 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution
871091 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #109873
871071 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #109835
870154 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #109503
869691 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #109418
867933 dotnet/runtime System.Collections.Immutable.Tests.WorkItemExecution #109714

Summary

24-Hour Hit Count 7-Day Hit Count 1-Month Count
0 11 14
@AndyAyersMS AndyAyersMS added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 12, 2024
@AndyAyersMS AndyAyersMS added this to the 10.0.0 milestone Nov 12, 2024
@AndyAyersMS AndyAyersMS mentioned this issue Nov 12, 2024
Merged
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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

@AndyAyersMS
Copy link
Member Author

@jakobbotsch possibly from the CSE/SSA change?

@AndyAyersMS AndyAyersMS changed the title JlIT: libraries jitstress Assertion failed 'doesVNMatch' during 'Assertion prop' JIT: libraries jitstress Assertion failed 'doesVNMatch' during 'Assertion prop' Nov 12, 2024
@jakobbotsch
Copy link
Member

Seems likely, I'll take a look

@jakobbotsch jakobbotsch self-assigned this Nov 13, 2024
@jakobbotsch jakobbotsch added the blocking-clean-ci-optional Blocking optional rolling runs label Nov 13, 2024
@jakobbotsch
Copy link
Member

jakobbotsch commented Nov 13, 2024 •
edited
Loading

CSEs are keyed by liberal VNs which guarantees that all defs and uses for a single CSE candidate have the same liberal VN. The same is not true for their conservative VNs, however -- each def/use may have a potentially unique conservative VN. However, when VN creates a new LCL_VAR, the VN it uses is just a random one from one of the other candidates -- it does not necessarily match the conservative VN that the reaching def has.

The fix will probably be to have the SSA inserter update the VN so that it matches the actual reaching definition.

@jakobbotsch
Copy link
Member

CSEs are keyed by liberal VNs which guarantees that all defs and uses for a single CSE candidate have the same liberal VN. The same is not true for their conservative VNs, however -- each def/use may have a potentially unique conservative VN. However, when VN creates a new LCL_VAR, the VN it uses is just a random one from one of the other candidates -- it does not necessarily match the conservative VN that the reaching def has.

The fix will probably be to have the SSA inserter update the VN so that it matches the actual reaching definition.

Things don't look quite that simple as CSE does a bit of post processing over the conservative VNs. Will probably have to refactor things in CSE a bit to now make use of the proper reaching definitions (as a side note, it looks like some things can be cleaned up now that we have this).

@jakobbotsch jakobbotsch added blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' Known Build Error Use this to report build issues in the .NET Helix tab and removed blocking-clean-ci-optional Blocking optional rolling runs labels Nov 14, 2024
@amanasifkhalid amanasifkhalid mentioned this issue Nov 18, 2024
Merged
jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Nov 19, 2024
@jakobbotsch
JIT: Update CSE to use reaching definition information
bcd1d0b 
Now that CSE always inserts into SSA we can update it to make use of the
reaching definition information that it has access to. CSE already spent
effort to track some extra information to try to do this, which we can
remove.

- Remove `optCSECheckedBoundMap`: this was used by CSE to try to update
  conservative VNs of ancestor bounds checks. This is unnececssary since
  all descendants of the CSE definitions should get the same
  conservative VNs automatically now.
- Remove `CSEdsc::defConservNormVN`; this was used to update
  conservative VNs in the single-def case, which again is unnecessary
  now

Making this change requires a bit of refactoring to the incremental SSA
builder. Before this PR the builder takes all defs and all uses and then
inserts everything into SSA. After this change the builder is used in a
multi-step process as follows:
1. All definitions are added with `IncrementalSsaBuilder::InsertDef`
2. The definitions are finalized with
   `IncrementalSsaBuilder::FinalizeDefs`
3. Uses are inserted (one by one) with
   `IncrementalSsaBuilder::InsertUse`. No finalization are necessary;
   each use is directly put into SSA as a result of calling this
   method.

The refactoring allows CSE to use the incremental SSA builder such that
it can access reaching definition information for each of the uses as
part of making replacements. However, this still requires some
refactoring such that CSE performs replacements of all defs before
performing the replacements of all uses.

Additionally, this PR fixes various incorrect VN updating made by CSE.

Fix dotnet#109745
jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Nov 19, 2024
@jakobbotsch
JIT: Update CSE to use reaching definition information
5abe998 
Now that CSE always inserts into SSA we can update it to make use of the
reaching definition information that it has access to. CSE already spent
effort to track some extra information to try to do this, which we can
remove.

- Remove `optCSECheckedBoundMap`: this was used by CSE to try to update
  conservative VNs of ancestor bounds checks. This is unnececssary since
  all descendants of the CSE definitions should get the same
  conservative VNs automatically now.
- Remove `CSEdsc::defConservNormVN`; this was used to update
  conservative VNs in the single-def case, which again is unnecessary
  now

Making this change requires a bit of refactoring to the incremental SSA
builder. Before this PR the builder takes all defs and all uses and then
inserts everything into SSA. After this change the builder is used in a
multi-step process as follows:
1. All definitions are added with `IncrementalSsaBuilder::InsertDef`
2. The definitions are finalized with
   `IncrementalSsaBuilder::FinalizeDefs`
3. Uses are inserted (one by one) with
   `IncrementalSsaBuilder::InsertUse`. No finalization are necessary;
   each use is directly put into SSA as a result of calling this
   method.

The refactoring allows CSE to use the incremental SSA builder such that
it can access reaching definition information for each of the uses as
part of making replacements. However, this still requires some
refactoring such that CSE performs replacements of all defs before
performing the replacements of all uses.

Additionally, this PR fixes various incorrect VN updating made by CSE.

VN and CSE still track VNs that are interesting bounds check. However,
VN was sometimes inserting VNs with exception sets into the set, which
is not useful (the consumers always use normal VNs when querying the
set). This PR fixes VN to insert the normal VN instead.

Fix dotnet#109745
jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Nov 19, 2024
@jakobbotsch
JIT: Use reaching definitions in CSE to update conservative VNs
dfe800f 
Now that CSE always inserts into SSA we can update it to make use of the
reaching definition information that it has access to. CSE already spent
effort to track some extra information to try to do this, which we can
remove.

- Remove `optCSECheckedBoundMap`: this was used by CSE to try to update
  conservative VNs of ancestor bounds checks. This is unnececssary since
  all descendants of the CSE definitions should get the same
  conservative VNs automatically now.
- Remove `CSEdsc::defConservNormVN`; this was used to update
  conservative VNs in the single-def case, which again is unnecessary
  now

Making this change requires a bit of refactoring to the incremental SSA
builder. Before this PR the builder takes all defs and all uses and then
inserts everything into SSA. After this change the builder is used in a
multi-step process as follows:
1. All definitions are added with `IncrementalSsaBuilder::InsertDef`
2. The definitions are finalized with
   `IncrementalSsaBuilder::FinalizeDefs`
3. Uses are inserted (one by one) with
   `IncrementalSsaBuilder::InsertUse`. No finalization are necessary;
   each use is directly put into SSA as a result of calling this
   method.

The refactoring allows CSE to use the incremental SSA builder such that
it can access reaching definition information for each of the uses as
part of making replacements. However, this still requires some
refactoring such that CSE performs replacements of all defs before
performing the replacements of all uses.

Additionally, this PR fixes various incorrect VN updating made by CSE.

VN and CSE still track VNs that are interesting bounds check. However,
VN was sometimes inserting VNs with exception sets into the set, which
is not useful (the consumers always use normal VNs when querying the
set). This PR fixes VN to insert the normal VN instead.

Fix dotnet#109745
@jakobbotsch jakobbotsch mentioned this issue Nov 19, 2024
Merged
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Nov 19, 2024
This was referenced Nov 19, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jakobbotsch jakobbotsch

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' in-pr There is an active PR which will close this issue when it is merged Known Build Error Use this to report build issues in the .NET Helix tab
Projects
None yet
Milestone
10.0.0
Development

Successfully merging a pull request may close this issue.

JIT: Use reaching definitions in CSE to update conservative VNs jakobbotsch/runtime
2 participants
@jakobbotsch @AndyAyersMS