Skip to content

Turn unreachable basic blocks into an infinite loop#98319

Merged
MichalStrehovsky merged 4 commits intodotnet:mainfrom
MichalStrehovsky:fix97758
Feb 14, 2024
Merged

Turn unreachable basic blocks into an infinite loop#98319
MichalStrehovsky merged 4 commits intodotnet:mainfrom
MichalStrehovsky:fix97758

Conversation

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Feb 12, 2024 •
edited
Loading

Fixes #97758.
Fixes #81339.

Instead of padding unreachable basic blocks with nops followed by br $-2, we now turn unreachable basic blocks into just br $-2, no nops (and matching basic block size) needed. This means offsets within the method body can shuffle around and we fix them up.

Cc @dotnet/ilc-contrib

@MichalStrehovsky
Don't allow creating 1-byte dead basic blocks
ef8155f 
Fixes dotnet#97758.

There's likely no substitution benefit from that (can't do anything meaningful in 1 byte), but this was causing the above issue: we had a mismatch for some IL patterns where we could optimize uninstantiated method bodies, but couldn't optimize instantiated ones because they ran into the abort at the bottom of this diff.
@MichalStrehovsky MichalStrehovsky self-assigned this Feb 12, 2024
@ghost
Copy link

ghost commented Feb 12, 2024

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

Issue Details

Fixes #97758.

There's likely no substitution benefit from that (can't do anything meaningful in 1 byte), but this was causing the above issue: we had a mismatch for some IL patterns where we could optimize uninstantiated method bodies, but couldn't optimize instantiated ones because they ran into the abort at the bottom of this diff.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: MichalStrehovsky
Labels:

area-NativeAOT-coreclr
Milestone: -

jkotas
jkotas approved these changes Feb 13, 2024
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

@jkotas
Copy link
Member

jkotas commented Feb 13, 2024

Is it worth it to add a test?

@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Feb 13, 2024

Is it worth it to add a test?

The regression test is quite non-sensical on it's own, but added anyway.

@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Feb 13, 2024

Hm, there's a test that ends up having this IL:

// [repro]FeatureCheckCombinations.MeetFeaturesEmptyIntersection(bool)
.method void MeetFeaturesEmptyIntersection(bool) cil managed
{
  // Code size: 53
  .maxstack 2
  .locals init (bool V_0,
      bool V_1,
      bool V_2)

  IL_0000:  nop
  IL_0001:  ldarg.0
  IL_0002:  stloc.0
  IL_0003:  ldloc.0
  IL_0004:  brfalse.s   IL_0018
  IL_0006:  nop
  IL_0007:  call        bool [System.Private.CoreLib]System.Runtime.CompilerServices.RuntimeFeature::get_IsDynamicCodeSupported()
  IL_000C:  ldc.i4.0
  IL_000D:  ceq
  IL_000F:  stloc.1
  IL_0010:  ldloc.1
  IL_0011:  brfalse.s   IL_0015
  IL_0013:  ldnull
  IL_0014:  throw
  IL_0015:  nop
  IL_0016:  br.s        IL_0028
  IL_0018:  nop
  IL_0019:  call        bool [System.Private.CoreLib]System.Runtime.CompilerServices.RuntimeFeature::get_IsDynamicCodeSupported()
  IL_001E:  ldc.i4.0
  IL_001F:  ceq
  IL_0021:  stloc.2
  IL_0022:  ldloc.2
  IL_0023:  brfalse.s   IL_0027
  IL_0025:  ldnull
  IL_0026:  throw
  IL_0027:  nop
  IL_0028:  call        void FeatureCheckCombinations::RequiresUnreferencedCode()
  IL_002D:  nop
  IL_002E:  call        void FeatureCheckCombinations::RequiresDynamicCode()
  IL_0033:  nop
  IL_0034:  ret
}

We now fail eliminating the part from IL_0027 because 0027 is a branch target and 0028 is a branch target and this logic ends up marking 0027 because it's too short.

The test has a point.

This substitution code is probably too garbage to fix this. It needs to be rewritten.

@build-analysis build-analysis bot mentioned this pull request Feb 13, 2024
Closed
@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Feb 14, 2024

I made a different fix that still avoids doing a rewrite.

Instead of padding unreachable basic blocks with nops followed by br $-2, we now turn unreachable basic blocks into just br $-2, no nops (and matching basic block size) needed. This means offsets within the method body can shuffle around and we fix them up.

This now also fixes #81339.

@MichalStrehovsky MichalStrehovsky changed the title Don't allow creating 1-byte dead basic blocks Turn unreachable basic blocks into an infinite loop Feb 14, 2024
@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Feb 14, 2024

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 14, 2024

Azure Pipelines successfully started running 1 pipeline(s).

jkotas
jkotas approved these changes Feb 14, 2024
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

This looks better

@MichalStrehovsky MichalStrehovsky merged commit fbe2b06 into dotnet:main Feb 14, 2024
@MichalStrehovsky MichalStrehovsky deleted the fix97758 branch February 14, 2024 22:24
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotas jkotas jkotas approved these changes

Assignees

@MichalStrehovsky MichalStrehovsky

Labels

area-NativeAOT-coreclr

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[NativeAOT] Assert in ILC when compiling System.Data.DataSetExtensions.Tests in Debug build Investigate removing nops from substitutions

2 participants

@MichalStrehovsky @jkotas