Skip to content

JIT: Fix some cases using BasicBlock::bbFallsThrough - #97699

Merged
jakobbotsch merged 1 commit into
dotnet:mainfrom
jakobbotsch:no-fallthrough-iv-fix
Jan 30, 2024
Merged

JIT: Fix some cases using BasicBlock::bbFallsThrough#97699
jakobbotsch merged 1 commit into
dotnet:mainfrom
jakobbotsch:no-fallthrough-iv-fix

Conversation

@jakobbotsch

@jakobbotschjakobbotsch commented Jan 30, 2024

Copy link
Copy Markdown
Member

bbFallsThrough still returns true for BBJ_COND; we have a couple of places using it as a "control flows from prev block" check, which is wrong after #97488.
Should fix the test failures I'm seeing over in #97182.

@jakobbotsch
JIT: Fix some cases using BasicBlock::bbFallsThrough
dfa5c00
`bbFallsThrough` still returns true for `BBJ_COND`; we have a couple of
places using it as a "control flows from prev block" check, which is
wrong after dotnet#97488.
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 30, 2024
@ghost

ghost commented Jan 30, 2024

Copy link
Copy Markdown

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

Issue Details

bbFallsThrough still returns true for BBJ_COND; we have a couple of places using it as a "control flows from prev block" check, which is wrong after #97488.
Should fix the test failures I'm seeing over in #97182.

Author:jakobbotsch
Assignees:jakobbotsch
Labels:

area-CodeGen-coreclr

Milestone:-

jakobbotsch
jakobbotsch commented Jan 30, 2024
View reviewed changes
Comment threadsrc/coreclr/jit/morph.cpp
Comment on lines +13889 to +13892
else
{
assertionsOut = pred->bbAssertionOut;
}

@jakobbotschjakobbotschJan 30, 2024

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Small drive-by fix for some UB here (in the useCondAssertions case we were accessing an inactive union member).

@ryujit-bot

ryujit-bot commented Jan 30, 2024

Copy link
Copy Markdown
Diff results for #97699

Assembly diffs

Assembly diffs for osx/arm64 ran on linux/x64

Diffs are based on 2,270,861 contexts (932,669 MinOpts, 1,338,192 FullOpts).

MISSED contexts: base: 1 (0.00%), diff: 9 (0.00%)

Overall (+684 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.osx.arm64.checked.mch34,437,860+60
benchmarks.run_tiered.osx.arm64.checked.mch15,516,128+208
coreclr_tests.run.osx.arm64.checked.mch486,460,784+172
libraries.crossgen2.osx.arm64.checked.mch55,725,496+84
libraries.pmi.osx.arm64.checked.mch80,219,060+72
libraries_tests.run.osx.arm64.Release.mch324,580,556+88
FullOpts (+684 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.osx.arm64.checked.mch18,136,188+60
benchmarks.run_tiered.osx.arm64.checked.mch4,011,424+208
coreclr_tests.run.osx.arm64.checked.mch153,807,100+172
libraries.crossgen2.osx.arm64.checked.mch55,723,868+84
libraries.pmi.osx.arm64.checked.mch80,097,932+72
libraries_tests.run.osx.arm64.Release.mch120,864,708+88

Assembly diffs for windows/arm64 ran on linux/x64

Diffs are based on 2,341,109 contexts (938,449 MinOpts, 1,402,660 FullOpts).

MISSED contexts: base: 0 (0.00%), diff: 8 (0.00%)

Overall (+668 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.windows.arm64.checked.mch45,573,924+60
benchmarks.run_tiered.windows.arm64.checked.mch15,587,008+208
coreclr_tests.run.windows.arm64.checked.mch495,312,136+172
libraries.crossgen2.windows.arm64.checked.mch59,070,264+84
libraries_tests.run.windows.arm64.Release.mch330,792,704+144
FullOpts (+668 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.windows.arm64.checked.mch29,562,156+60
benchmarks.run_tiered.windows.arm64.checked.mch4,409,600+208
coreclr_tests.run.windows.arm64.checked.mch156,582,272+172
libraries.crossgen2.windows.arm64.checked.mch59,068,628+84
libraries_tests.run.windows.arm64.Release.mch127,359,108+144

Assembly diffs for windows/x64 ran on linux/x64

Diffs are based on 2,512,204 contexts (997,391 MinOpts, 1,514,813 FullOpts).

MISSED contexts: base: 0 (0.00%), diff: 8 (0.00%)

Overall (+630 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.windows.x64.checked.mch36,236,339+68
benchmarks.run_tiered.windows.x64.checked.mch12,415,933+178
coreclr_tests.run.windows.x64.checked.mch393,193,287+152
libraries.crossgen2.windows.x64.checked.mch39,486,146+56
libraries_tests.run.windows.x64.Release.mch282,112,885+176
FullOpts (+630 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.windows.x64.checked.mch22,065,683+68
benchmarks.run_tiered.windows.x64.checked.mch3,316,694+178
coreclr_tests.run.windows.x64.checked.mch120,404,433+152
libraries.crossgen2.windows.x64.checked.mch39,484,959+56
libraries_tests.run.windows.x64.Release.mch106,254,567+176

Details here

Throughput diffs

Throughput diffs for osx/arm64 ran on windows/x64

Overall (-0.01% to +0.00%)
CollectionPDIFF
benchmarks.run.osx.arm64.checked.mch-0.01%
FullOpts (-0.01% to +0.00%)
CollectionPDIFF
benchmarks.run.osx.arm64.checked.mch-0.01%

Throughput diffs for windows/arm64 ran on windows/x64

Overall (-0.01% to +0.00%)
CollectionPDIFF
benchmarks.run.windows.arm64.checked.mch-0.01%
FullOpts (-0.01% to +0.00%)
CollectionPDIFF
benchmarks.run.windows.arm64.checked.mch-0.01%

Throughput diffs for windows/x64 ran on windows/x64

Overall (-0.01% to +0.00%)
CollectionPDIFF
benchmarks.run.windows.x64.checked.mch-0.01%
FullOpts (-0.01% to +0.00%)
CollectionPDIFF
benchmarks.run.windows.x64.checked.mch-0.01%

Details here

@ryujit-bot

ryujit-bot commented Jan 30, 2024

Copy link
Copy Markdown
Diff results for #97699

Assembly diffs

Assembly diffs for linux/arm ran on windows/x86

Diffs are based on 2,239,391 contexts (829,328 MinOpts, 1,410,063 FullOpts).

MISSED contexts: 71,273 (3.08%)

Overall (+52 bytes)
CollectionBase size (bytes)Diff size (bytes)
libraries.crossgen2.linux.arm.checked.mch34,522,542+52
FullOpts (+52 bytes)
CollectionBase size (bytes)Diff size (bytes)
libraries.crossgen2.linux.arm.checked.mch34,521,312+52

Assembly diffs for windows/x86 ran on windows/x86

Diffs are based on 2,293,495 contexts (839,658 MinOpts, 1,453,837 FullOpts).

MISSED contexts: 1 (0.00%)

Overall (+46 bytes)
CollectionBase size (bytes)Diff size (bytes)
libraries.crossgen2.windows.x86.checked.mch31,716,932+46
FullOpts (+46 bytes)
CollectionBase size (bytes)Diff size (bytes)
libraries.crossgen2.windows.x86.checked.mch31,715,872+46

Details here

Assembly diffs for linux/arm64 ran on windows/x64

Diffs are based on 2,507,310 contexts (1,007,092 MinOpts, 1,500,218 FullOpts).

MISSED contexts: base: 0 (0.00%), diff: 8 (0.00%)

Overall (+584 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.linux.arm64.checked.mch80,094,172+60
benchmarks.run_tiered.linux.arm64.checked.mch24,600,952+208
coreclr_tests.run.linux.arm64.checked.mch508,772,940+172
libraries.crossgen2.linux.arm64.checked.mch55,844,024+84
libraries_tests.run.linux.arm64.Release.mch395,688,768+60
FullOpts (+584 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.linux.arm64.checked.mch54,159,016+60
benchmarks.run_tiered.linux.arm64.checked.mch4,862,296+208
coreclr_tests.run.linux.arm64.checked.mch160,584,028+172
libraries.crossgen2.linux.arm64.checked.mch55,842,388+84
libraries_tests.run.linux.arm64.Release.mch180,556,816+60

Assembly diffs for linux/x64 ran on windows/x64

Diffs are based on 2,517,901 contexts (991,070 MinOpts, 1,526,831 FullOpts).

MISSED contexts: base: 0 (0.00%), diff: 8 (0.00%)

Overall (+567 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.linux.x64.checked.mch71,590,553+51
benchmarks.run_tiered.linux.x64.checked.mch21,435,525+218
coreclr_tests.run.linux.x64.checked.mch403,710,934+133
libraries.crossgen2.linux.x64.checked.mch38,727,133+59
libraries_tests.run.linux.x64.Release.mch337,107,837+106
FullOpts (+567 bytes)
CollectionBase size (bytes)Diff size (bytes)
benchmarks.run_pgo.linux.x64.checked.mch47,790,564+51
benchmarks.run_tiered.linux.x64.checked.mch3,694,745+218
coreclr_tests.run.linux.x64.checked.mch123,956,232+133
libraries.crossgen2.linux.x64.checked.mch38,725,935+59
libraries_tests.run.linux.x64.Release.mch153,348,144+106

Details here

Throughput diffs

Throughput diffs for linux/arm64 ran on linux/x64

Overall (-0.01% to -0.00%)
CollectionPDIFF
realworld.run.linux.arm64.checked.mch-0.01%
smoke_tests.nativeaot.linux.arm64.checked.mch-0.01%
libraries.pmi.linux.arm64.checked.mch-0.01%
benchmarks.run_pgo.linux.arm64.checked.mch-0.01%
benchmarks.run.linux.arm64.checked.mch-0.01%
libraries_tests.run.linux.arm64.Release.mch-0.01%
FullOpts (-0.01% to -0.00%)
CollectionPDIFF
realworld.run.linux.arm64.checked.mch-0.01%
smoke_tests.nativeaot.linux.arm64.checked.mch-0.01%
libraries.pmi.linux.arm64.checked.mch-0.01%
benchmarks.run_pgo.linux.arm64.checked.mch-0.01%
benchmarks.run.linux.arm64.checked.mch-0.01%
libraries_tests.run.linux.arm64.Release.mch-0.01%

Throughput diffs for linux/x64 ran on linux/x64

Overall (-0.01% to -0.00%)
CollectionPDIFF
benchmarks.run.linux.x64.checked.mch-0.01%
realworld.run.linux.x64.checked.mch-0.01%
benchmarks.run_pgo.linux.x64.checked.mch-0.01%
libraries_tests.run.linux.x64.Release.mch-0.01%
libraries_tests_no_tiered_compilation.run.linux.x64.Release.mch-0.01%
smoke_tests.nativeaot.linux.x64.checked.mch-0.01%
libraries.pmi.linux.x64.checked.mch-0.01%
libraries.crossgen2.linux.x64.checked.mch-0.01%
FullOpts (-0.01% to -0.00%)
CollectionPDIFF
benchmarks.run.linux.x64.checked.mch-0.01%
realworld.run.linux.x64.checked.mch-0.01%
coreclr_tests.run.linux.x64.checked.mch-0.01%
benchmarks.run_pgo.linux.x64.checked.mch-0.01%
libraries_tests.run.linux.x64.Release.mch-0.01%
libraries_tests_no_tiered_compilation.run.linux.x64.Release.mch-0.01%
smoke_tests.nativeaot.linux.x64.checked.mch-0.01%
libraries.pmi.linux.x64.checked.mch-0.01%
libraries.crossgen2.linux.x64.checked.mch-0.01%

Details here

@jakobbotsch

jakobbotsch commented Jan 30, 2024

Copy link
Copy Markdown
MemberAuthor

cc @dotnet/jit-contrib PTAL @amanasifkhalid

Diffs. A few cases where we now recognize an induction due to optExtractInitTestIncr being more general, and then start loop cloning.
I left bbFallsThrough alone since I wasn't sure if we want to change its behavior yet (noticed a few cases that explicitly check it and then filter out BBJ_COND cases).

@jakobbotsch
jakobbotsch marked this pull request as ready for review January 30, 2024 17:55
@build-analysisbuild-analysisBot mentioned this pull request Jan 30, 2024
Open
3 tasks
@amanasifkhalid

amanasifkhalid commented Jan 30, 2024

Copy link
Copy Markdown
Contributor

Thanks for the fix!

I left bbFallsThrough alone since I wasn't sure if we want to change its behavior yet (noticed a few cases that explicitly check it and then filter out BBJ_COND cases).

My hope is that we can get rid of bbFallsThrough altogether soon, and replace it with a pattern like the unique predecessor check you're using here. Those explicit BBJ_COND checks were probably introduced as quirks to reduce diffs for now.

@jakobbotsch
jakobbotsch merged commit a3485bd into dotnet:mainJan 30, 2024
@jakobbotsch
jakobbotsch deleted the no-fallthrough-iv-fix branch January 30, 2024 18:46
@build-analysisbuild-analysisBot mentioned this pull request Jan 31, 2024
Closed
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 12, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@amanasifkhalidamanasifkhalidamanasifkhalid approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@jakobbotschjakobbotsch

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jakobbotsch@ryujit-bot@amanasifkhalid