Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

JIT: fix assert when there are mixed types in Enum.HasFlags optimization #23902

Merged

Conversation

AndyAyersMS
Copy link
Member

In some cases the pre-boxed nodes may differ in type. Bail if they don't have
the same stack type, then compute the result using the stack type.

Extended the hasflags test with a case that shows this issue.

Closes #23847

In some cases the pre-boxed nodes may differ in type. Bail if they don't have
the same stack type, then compute the result using the stack type.

Extended the hasflags test with a case that shows this issue.

Closes #23847
@AndyAyersMS
Copy link
Member Author

cc @dotnet/jit-contrib

Two diffs in FX where we now and/cmp int instead of short:

PMI Diffs for System.Private.CoreLib.dll, framework assemblies for x64 default jit
Summary:
(Lower is better)
Total bytes of diff: -8 (-0.00% of base)
    diff is an improvement.
Top file improvements by size (bytes):
          -8 : NuGet.LibraryModel.dasm (-0.04% of base)
1 total files with size differences (1 improved, 0 regressed), 128 unchanged.
Top method improvements by size (bytes):
          -4 (-0.73% of base) : NuGet.LibraryModel.dasm - LibraryDependencyTargetUtils:GetFlagString(ushort):ref
          -4 (-0.73% of base) : NuGet.LibraryModel.dasm - LibraryIncludeFlagUtils:GetFlagString(ushort):ref
Top method improvements by size (percentage):
          -4 (-0.73% of base) : NuGet.LibraryModel.dasm - LibraryDependencyTargetUtils:GetFlagString(ushort):ref
          -4 (-0.73% of base) : NuGet.LibraryModel.dasm - LibraryIncludeFlagUtils:GetFlagString(ushort):ref
2 total methods with size differences (2 improved, 0 regressed), 200142 unchanged.

Copy link

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AndyAyersMS
Copy link
Member Author

CoreFX had one test timeout:

   System.Net.Security.Tests.SslStreamCredentialCacheTest.SslStream_SameCertUsedForClientAndServer_Ok [FAIL]
11:26:41       System.TimeoutException : Task timed out after 15000ms
``

@AndyAyersMS
Copy link
Member Author

musl failure looks like some kind of CI issue:

  Job 11c4e90e-6213-45c1-9b0a-e3b0b5e6d62a is completed with 23 finished work items.
  Stopping Azure Pipelines Test Run Linux x64 Release no_tiered_compilation @ (Alpine.38.Amd64.Open)Ubuntu.1604.Amd64.Open@mcr.microsoft.com/dotnet-buildtools/prereqs:alpine-3.8-helix-45b1fa2-20190327215821
  Job d0675952-3aaf-4ccf-8e98-411f4437b7e4 is completed with 23 finished work items.
  Stopping Azure Pipelines Test Run Linux x64 Release @ (Alpine.38.Amd64.Open)Ubuntu.1604.Amd64.Open@mcr.microsoft.com/dotnet-buildtools/prereqs:alpine-3.8-helix-45b1fa2-20190327215821
/home/helixbot_azpcontainer/.nuget/packages/microsoft.dotnet.helix.sdk/2.0.0-beta.19210.7/tools/Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(67,5): error : This task can only be used on completed jobs. There are 1 of 23 unfinished work items. [/__w/1/s/tests/helixpublishwitharcade.proj]

Build FAILED.

@echesakov
Copy link

musl failure looks like some kind of CI issue:
Job 11c4e90e-6213-45c1-9b0a-e3b0b5e6d62a is completed with 23 finished work items.
Stopping Azure Pipelines Test Run Linux x64 Release no_tiered_compilation @ (Alpine.38.Amd64.Open)Ubuntu.1604.Amd64.Open@mcr.microsoft.com/dotnet-buildtools/prereqs:alpine-3.8-helix-45b1fa2-20190327215821
Job d0675952-3aaf-4ccf-8e98-411f4437b7e4 is completed with 23 finished work items.
Stopping Azure Pipelines Test Run Linux x64 Release @ (Alpine.38.Amd64.Open)Ubuntu.1604.Amd64.Open@mcr.microsoft.com/dotnet-buildtools/prereqs:alpine-3.8-helix-45b1fa2-20190327215821
/home/helixbot_azpcontainer/.nuget/packages/microsoft.dotnet.helix.sdk/2.0.0-beta.19210.7/tools/Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(67,5): error : This task can only be used on completed jobs. There are 1 of 23 unfinished work items. [/__w/1/s/tests/helixpublishwitharcade.proj]

Build FAILED.

@AndyAyersMS Yes, I suspect it's https://github.com/dotnet/core-eng/issues/5883

@AndyAyersMS
Copy link
Member Author

Failures seem unrelated, so am going to merge this.

@AndyAyersMS AndyAyersMS merged commit 759e377 into dotnet:master Apr 11, 2019
@AndyAyersMS AndyAyersMS deleted the FixEnumHasFlagOptMixedTypeIssue branch April 11, 2019 23:45
davmason pushed a commit to davmason/coreclr that referenced this pull request Apr 27, 2019
…ion (dotnet#23902)

In some cases the pre-boxed nodes may differ in type. Bail if they don't have
the same stack type, then compute the result using the stack type.

Extended the hasflags test with a case that shows this issue.

Closes #23847
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…ion (dotnet/coreclr#23902)

In some cases the pre-boxed nodes may differ in type. Bail if they don't have
the same stack type, then compute the result using the stack type.

Extended the hasflags test with a case that shows this issue.

Closes dotnet/coreclr#23847

Commit migrated from dotnet/coreclr@759e377
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion failed 'type == flagVal->TypeGet()'
4 participants