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.

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

Enable StructEnreg by default on all platforms. #55558

Merged
merged 4 commits into from
Jul 15, 2021

Conversation

sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Jul 13, 2021

Diffs are positive and looks similar to x64 diffs merged in #55045 and x86 merged in #55535.

coreclr_tests.pmi.Linux.arm64.checked.mch
Total bytes of delta: -3112
153 total files with Code Size differences (151 improved, 2 regressed), 7 unchanged.

libraries.crossgen2.Linux.arm64.checked.mch
Total bytes of delta: -4460
561 total files with Code Size differences (462 improved, 99 regressed), 49 unchanged.

libraries.pmi.Linux.arm64.checked.mch
Total bytes of delta: -5300
111 total files with Code Size differences (95 improved, 16 regressed), 11 unchanged.

libraries_tests.pmi.Linux.arm64.checked.mch
Total bytes of delta: -324
85 total files with Code Size differences (58 improved, 27 regressed), 17 unchanged.

benchmarks.run.Linux.x64.checked.mch
Total bytes of delta: -390
5 total files with Code Size differences (5 improved, 0 regressed), 0 unchanged.

coreclr_tests.pmi.Linux.x64.checked.mch
Total bytes of delta: -4996
157 total files with Code Size differences (156 improved, 1 regressed), 4 unchanged.

libraries.crossgen2.Linux.x64.checked.mch
Total bytes of delta: -3153
271 total files with Code Size differences (224 improved, 47 regressed), 18 unchanged.

libraries.pmi.Linux.x64.checked.mch
Total bytes of delta: -970
80 total files with Code Size differences (68 improved, 12 regressed), 0 unchanged.

Total bytes of delta: -997
libraries_tests.pmi.Linux.x64.checked.mch
85 total files with Code Size differences (69 improved, 16 regressed), 4 unchanged.

benchmarks.run.windows.arm64.checked.mch
Total bytes of delta: -160
5 total files with Code Size differences (5 improved, 0 regressed), 0 unchanged.

coreclr_tests.pmi.windows.arm64.checked.mch
Total bytes of delta: -3120
153 total files with Code Size differences (151 improved, 2 regressed), 7 unchanged.

libraries.crossgen2.windows.arm64.checked.mch
Total bytes of delta: -4536
572 total files with Code Size differences (472 improved, 100 regressed), 48 unchanged.

libraries.pmi.windows.arm64.checked.mch
Total bytes of delta: -5376 (-5.36% of base)
118 total files with Code Size differences (102 improved, 16 regressed), 11 unchanged.

libraries_tests.pmi.windows.arm64.checked.mch
Total bytes of delta: -308
85 total files with Code Size differences (56 improved, 29 regressed), 18 unchanged.

coreclr_tests.pmi.Linux.arm.checked.mch
Total bytes of delta: -1660
49 total files with Code Size differences (48 improved, 1 regressed), 0 unchanged.

libraries.crossgen2.Linux.arm.checked.mch
Total bytes of delta: -1280
208 total files with Code Size differences (185 improved, 23 regressed), 13 unchanged.

libraries.pmi.Linux.arm.checked.mch
Total bytes of delta: -34
3 total methods with Code Size differences (2 improved, 1 regressed), 0 unchanged.

libraries_tests.pmi.Linux.arm.checked.mch
Total bytes of delta: -118
7 total files with Code Size differences (7 improved, 0 regressed), 4 unchanged.

The regressions are the same:

  1. we use different frame size and it could require bigger init code;
  2. we use more registers and have to store/restore non-volatile.

The fixes that are included in this PR:

  1. genBitCast on arm64/arm32 was not working correctly with regOptional lclVars, did not repro without additional stress pressure, the bug was that if a local was not assigned a register and we had a bitcast on top of it we were marking it as contained and were trying to load directly but for locals the direct load happens in genConsumeRegs that has already happened there and it was skipped because the local was contained;
  2. arm32 prespill all struct args, so they are excluded from register allocation;
  3. SIMDIntrinsicUpperSave/Restore for SIMD regs when calling conv does not keep them, so if we enregister struct<16> as v8 we have to do bookkeeping for the upper part around each call etc.

the problem with these fixes was that they all were silent bad codegen, I have covered the 3 with new asserts but others are too low in the pipeline to be checked.

Contributes to #43867 and probably closes the scope for 6.0.

@sandreenko sandreenko added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 13, 2021
@sandreenko
Copy link
Contributor Author

/azp run runtime-coreclr jitstress, runtime-coreclr outerloop, runtime-coreclr jitstressregs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@sandreenko
Copy link
Contributor Author

/azp run runtime-coreclr jitstress, runtime-coreclr outerloop, runtime-coreclr jitstressregs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@sandreenko
Copy link
Contributor Author

/azp run runtime-coreclr jitstress2-jitstressregs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sergey added 3 commits July 14, 2021 01:32
fix arm32

Fix arm/arm64.

now we can have contained lclRead for other platforms, not only xarch.
@sandreenko
Copy link
Contributor Author

/azp run runtime-coreclr jitstress2-jitstressregs, runtime-coreclr jitstress, runtime-coreclr outerloop, runtime-coreclr jitstressregs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@sandreenko sandreenko changed the title Test Enable StructEnreg by default on all platforms. Jul 14, 2021
@sandreenko sandreenko marked this pull request as ready for review July 14, 2021 09:08
@sandreenko
Copy link
Contributor Author

PTAL @dotnet/jit-contrib

@kunalspathak
Copy link
Member

Does this fix ARM64 issue #35071?

lvaSetVarDoNotEnregister(lclNum DEBUGARG(DNER_IsStructArg));
}
}
else if (varDsc->lvType == TYP_STRUCT)
Copy link
Member

Choose a reason for hiding this comment

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

This is the same condition as before; this block appears to be dead code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like a merge issue and that is why I am seeing some failures in CI, will fix it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thank you.

@sandreenko
Copy link
Contributor Author

/azp run runtime-coreclr jitstress2-jitstressregs, runtime-coreclr jitstress, runtime-coreclr outerloop, runtime-coreclr jitstressregs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@sandreenko sandreenko merged commit 90b8ddd into dotnet:main Jul 15, 2021
@sandreenko
Copy link
Contributor Author

The UI here does not show the test result (yeah, ci) but:

  1. runtime is green;
  2. runtime-jitstress is green
  3. runtime jitstressregs only one unrelated failure, covered by Exception in test infrastructure - System.InvalidOperationException: Collection was modified after the enumerator was instantiated #11063 (comment)
  4. runtime jitstress2-jitstressregs shows one unrelated failure covered by Feed unreliability affecting CI #55449 (comment);

so the tests are passing, I will keep an eye on library stress tests but we have time to fix bugs if anything pops up! Thank Bruce again for the quick review.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants