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

Add logic for selecting the right version of pgort140 #71968

Merged
merged 4 commits into from
Jul 12, 2022

Conversation

DrewScoggins
Copy link
Member

This fixes an issue where the Arm64 PGO instrumented SDK had the x64 version of pgort140.dll

@DrewScoggins DrewScoggins requested review from agocke and a team July 11, 2022 19:14
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 11, 2022
@ghost ghost assigned DrewScoggins Jul 11, 2022
@ghost
Copy link

ghost commented Jul 11, 2022

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

Issue Details

This fixes an issue where the Arm64 PGO instrumented SDK had the x64 version of pgort140.dll

Author: DrewScoggins
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@DrewScoggins
Copy link
Member Author

/azp run "CoreCLR Product Build windows arm64 release PGO"

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@DrewScoggins
Copy link
Member Author

I have already tested this on the PGO builds locally, so if all the other checks pass I would like to merge this ASAP. @agocke @dotnet/runtime-infrastructure

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM aside from what looks like dead code

src/coreclr/jit/CMakeLists.txt Outdated Show resolved Hide resolved
if(CLR_CMAKE_TARGET_ARCH_ARM64)
cmake_path(SET PGORT_DIR "$ENV{VCToolsInstallDir}bin/arm64")
elseif(CLR_CMAKE_TARGET_ARCH_I386)
cmake_path(SET PGORT_DIR "$ENV{VCToolsInstallDir}bin/Hostx86/x86")
Copy link
Member

Choose a reason for hiding this comment

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

This should probably work, but I think we only support building on x64, so technically this should be Hostx64/x86

Copy link
Member Author

Choose a reason for hiding this comment

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

I will fix it up. Want to do it right :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, Hostx64/x86 does not contain pgort140.dll. So I am going to stick with what I have.

@mrsharm
Copy link
Member

mrsharm commented Aug 10, 2022

We detected that might be caused by this PR via the August Perf Report and looks like it was reported in a few places but hasn't been triaged yet:

Here are the details from the perf report:

System.Numerics.Tests.Perf_BigInteger.Parse(numberString: 12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890)

Result Ratio Alloc Delta Operating System Bit Processor Name
Slower 0.72 +0 Windows 11 Arm64 Microsoft SQ1 3.0 GHz
Slower 0.72 +0 Windows 11 Arm64 Microsoft SQ1 3.0 GHz
Slower 0.77 +0 macOS Monterey 12.3 Arm64 Apple M1 Max
Same 0.92 +0 Windows 10 X64 Intel Xeon CPU E5-1650 v4 3.60GHz
Same 0.92 +0 Windows 10 X64 Intel Core i7-6700 CPU 3.40GHz (Skylake)
Same 0.90 +0 Windows 10 X64 Intel Core i7-6700 CPU 3.40GHz (Skylake)
Same 0.90 +0 Windows 10 X64 Intel Core i7-8650U CPU 1.90GHz (Kaby Lake R)
Same 0.93 +0 Windows 10 X64 Intel Core i9-10900K CPU 3.70GHz
Slower 0.85 +0 Windows 11 X64 AMD Ryzen Threadripper PRO 3945WX 12-Cores
Slower 0.88 +0 Windows 11 X64 AMD Ryzen 9 3950X
Slower 0.89 +0 Windows 11 X64 AMD Ryzen 9 5900X
Same 0.90 +0 Windows 11 X64 AMD Ryzen 9 5950X
Slower 0.89 +0 Windows 11 X64 Intel Core i7-8700 CPU 3.20GHz (Coffee Lake)
Slower 0.89 +0 Windows 11 X64 Intel Core i9-10900K CPU 3.70GHz
Same 0.93 +0 Windows 11 X64 11th Gen Intel Core i9-11900H 2.50GHz
Same 1.01 +0 ubuntu 18.04 X64 Intel Xeon CPU E5-1650 v4 3.60GHz
Same 0.99 +0 ubuntu 18.04 X64 Intel Core i7-2720QM CPU 2.20GHz (Sandy Bridge)
Slower 0.84 +0 ubuntu 18.04 X64 Intel Core i7-8700 CPU 3.20GHz (Coffee Lake)
Slower 0.86 +0 ubuntu 20.04 X64 AMD Ryzen 9 5900X
Slower 0.80 +0 ubuntu 20.04 X64 Intel Core i9-10900K CPU 3.70GHz
Slower 0.85 +0 Windows 10 X86 Intel Xeon CPU E5-1650 v4 3.60GHz
Slower 0.85 +0 Windows 10 X86 Intel Core i7-6700 CPU 3.40GHz (Skylake)
Slower 0.82 +0 Windows 11 X86 AMD Ryzen Threadripper PRO 3945WX 12-Cores
Slower 0.83 +0 macOS Big Sur 11.6.8 X64 Intel Core i5-4278U CPU 2.60GHz (Haswell)
Slower 0.77 +0 macOS Monterey 12.3.1 X64 Intel Core i7-5557U CPU 3.10GHz (Broadwell)
Slower 0.86 +0 macOS Monterey 12.4 X64 Intel Core i5-4278U CPU 2.60GHz (Haswell)

@DrewScoggins
Copy link
Member Author

I doubt that this change could have caused this regression. It does not change anything about the shipping product, it just changes one of the binaries that gets bin-placed for the instrumented SDK.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 17, 2022
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