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

Mark vars as do not enreg earlier in minopts. #54998

Merged
merged 4 commits into from
Jul 1, 2021

Conversation

sandreenko
Copy link
Contributor

@sandreenko sandreenko commented Jul 1, 2021

and improve morph block init/copy a little bit.

There is a known problem that we start asking is it already known to be not on register? before the value is finalized during morph (block copy, should we do field by field) and lowering (do we need bitcast? can we mark the source as contained?) .
I have an markdown description of it in 6.0 planning.

For now, just mark them as doNotEnreg earlier when enregistration is disabled. Give us small improvements on minopts methods and prevent some regressions with future changes.

coreclr_tests.pmi.windows.x64.checked.mch
Total bytes of delta: -4
1 total methods with Code Size differences (1 improved, 0 regressed)

coreclr_tests.pmi.Linux.arm64.checked.mch
Total bytes of delta: -8
2 total methods with Code Size differences (2 improved, 0 regressed)

libraries_tests.pmi.Linux.arm64.checked.mch
Total bytes of delta: -28
5 total methods with Code Size differences (5 improved, 0 regressed)

benchmarks.run.Linux.x64.checked.mch
Total bytes of delta: -3
1 total methods with Code Size differences (1 improved, 0 regressed)

coreclr_tests.pmi.Linux.x64.checked.mch
Total bytes of delta: -4 
1 total methods with Code Size differences (1 improved, 0 regressed)

benchmarks.run.windows.arm64.checked.mch
Total bytes of delta: -8
1 total methods with Code Size differences (1 improved, 0 regressed)

coreclr_tests.pmi.windows.arm64.checked.mch
Total bytes of delta: -8
2 total methods with Code Size differences (2 improved, 0 regressed)

libraries_tests.pmi.windows.arm64.checked.mch
Total bytes of delta: -28
5 total methods with Code Size differences (5 improved, 0 regressed)

coreclr_tests.pmi.windows.x86.checked.mch
Total bytes of delta: -4
1 total methods with Code Size differences (1 improved, 0 regressed)

+no diffs improvements of morph block init/copy.

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

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

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).


// Mark the dest/src structs as DoNotEnreg when they are not being fully referenced as the same type.
//
if (!m_dstDoFldAsg && (m_dstVarDsc != nullptr) && !m_dstSingleLclVarAsg)
Copy link
Contributor Author

@sandreenko sandreenko Jul 1, 2021

Choose a reason for hiding this comment

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

after this change init/copy like:

           [000015] -ACXG-------              *  ASG       struct (copy)
           [000013] D----+-N----              +--*  LCL_VAR   struct<S, 8> V01 loc0
           [000012] --CXG+------              \--*  CALL      struct TestStructFields.Program.GetS1

don't require doNotEnreg flag. The condition was moved after we do m_comp->fgMorphBlockOperand to work after OBJ(ADDR(LCL)) folding.

@@ -3467,7 +3473,7 @@ void Compiler::lvaSortByRefCount()
assert(varDsc->lvType != TYP_STRUCT ||
varDsc->lvDoNotEnregister); // For structs, should have set this when we set lvAddrExposed.
}
else if (varTypeIsStruct(varDsc))
if (varTypeIsStruct(varDsc))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the idea of if/else was that each block was setting lvaSetVarDoNotEnregister and there was no need to check the rest, now we have blocks that don't set it, so we need to check each.

@sandreenko
Copy link
Contributor Author

PTAL @BruceForstall @dotnet/jit-contrib , another preparation for structs in registers, small changes and diffs.

@sandreenko sandreenko merged commit eeadfdb into dotnet:main Jul 1, 2021
@kunalspathak
Copy link
Member

kunalspathak commented Jul 6, 2021

@EgorBo
Copy link
Member

EgorBo commented Jul 6, 2021

Simplest repro:

cd src\benchmarks\micro
dotnet run -c Release -f net5.0 -- --coreRun C:\prj\runtime\artifacts\tests\coreclr\windows.x64.Release\Tests\Core_Root\corerun.exe --filter "*IniArray.Test*" 

It prints around 179ms, then if I revert changes from this PR by hands and re-run - it prints 45ms. The benchmark itself seems to have the same codegen so I have no clue what's going on...

@sandreenko
Copy link
Contributor Author

@sandreenko - We noticed several regressions possibly related to this change. Do you mind going through some of them to confirm if they are indeed from this change?

Sure, looking at them now, thanks for reporting it.

@sandreenko
Copy link
Contributor Author

dotnet run -c Release -f net5.0 -- --coreRun C:\prj\runtime\artifacts\tests\coreclr\windows.x64.Release\Tests\Core_Root\corerun.exe --filter "*IniArray.Test*" 

Hm, don't see this:

D:\Sergey\git\runtime\.dotnet\dotnet.exe run -c Release -f net5.0 --  --cli D:\Sergey\git\runtime\.dotnet\dotnet.exe  --filter "*IniArray.Test*" --coreRun "D:\Sergey\git\runtime\artifacts\tests\coreclr\windows.x64.Release\Tests\Core_Root_base\corerun.exe"

|   Method |     Mean |    Error |   StdDev |   Median |      Min |      Max | Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------- |---------:|---------:|---------:|---------:|---------:|---------:|------:|------:|------:|----------:|
| IniArray | 42.86 ms | 0.826 ms | 0.690 ms | 42.56 ms | 42.33 ms | 44.75 ms |     - |     - |     - |      88 B |

D:\Sergey\git\runtime\.dotnet\dotnet.exe run -c Release -f net5.0 --  --cli D:\Sergey\git\runtime\.dotnet\dotnet.exe  --filter "*IniArray.Test*" --coreRun "D:\Sergey\git\runtime\artifacts\tests\coreclr\windows.x64.Release\Tests\Core_Root_diff\corerun.exe"
|   Method |     Mean |    Error |   StdDev |   Median |      Min |      Max | Gen 0 | Gen 1 | Gen 2 | Allocated |
|--------- |---------:|---------:|---------:|---------:|---------:|---------:|------:|------:|------:|----------:|
| IniArray | 42.57 ms | 0.175 ms | 0.136 ms | 42.58 ms | 42.35 ms | 42.80 ms |     - |     - |     - |      88 B |

I will check other regressions.

@EgorBo
Copy link
Member

EgorBo commented Jul 6, 2021

@sandreenko I've just checked on a clean repo again - still reproduces for me

@EgorBo
Copy link
Member

EgorBo commented Jul 6, 2021

@sandreenko managed to get the diff between two coreruns: https://www.diffchecker.com/qh9EOTD4

@sandreenko
Copy link
Contributor Author

@sandreenko managed to get the diff between two coreruns: https://www.diffchecker.com/qh9EOTD4

Thank you, now I see them, I had a system-wide tiering disabled that I forgot about.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 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.

4 participants