-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Fix redhat arm64 #52244
Fix redhat arm64 #52244
Changes from all commits
fef1a9e
285cada
9970b60
3bd6fbf
0433807
1ea2887
9bfa3a0
99aa111
09c5999
f1abcd3
e66eb50
33bb47c
3d1e65d
5f0ff6d
bad50b6
0c4e740
3856661
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,10 @@ function(add_pgo TargetName) | |
else(CLR_CMAKE_HOST_WIN32) | ||
if(UPPERCASE_CMAKE_BUILD_TYPE STREQUAL RELEASE OR UPPERCASE_CMAKE_BUILD_TYPE STREQUAL RELWITHDEBINFO) | ||
target_compile_options(${TargetName} PRIVATE -flto -fprofile-instr-generate) | ||
set_property(TARGET ${TargetName} APPEND_STRING PROPERTY LINK_FLAGS " -flto -fuse-ld=gold -fprofile-instr-generate") | ||
set_property(TARGET ${TargetName} APPEND_STRING PROPERTY LINK_FLAGS " -flto -fprofile-instr-generate") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to confirm, LLD supports the same PGO linker flags as GOLD, correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct. I have verified that a release build with -pgoinstrument passes and that the compiler was really passed these options. |
||
if(NOT LD_LLVM) | ||
set_property(TARGET ${TargetName} APPEND_STRING PROPERTY LINK_FLAGS " -fuse-ld=gold") | ||
endif() | ||
endif(UPPERCASE_CMAKE_BUILD_TYPE STREQUAL RELEASE OR UPPERCASE_CMAKE_BUILD_TYPE STREQUAL RELWITHDEBINFO) | ||
endif(CLR_CMAKE_HOST_WIN32) | ||
elseif(CLR_CMAKE_PGO_OPTIMIZE) | ||
|
@@ -54,7 +57,10 @@ function(add_pgo TargetName) | |
if((CMAKE_CXX_COMPILER_ID MATCHES "Clang") AND (NOT CMAKE_CXX_COMPILER_VERSION VERSION_LESS 3.6)) | ||
if(HAVE_LTO) | ||
target_compile_options(${TargetName} PRIVATE -flto -fprofile-instr-use=${ProfilePath} -Wno-profile-instr-out-of-date -Wno-profile-instr-unprofiled) | ||
set_property(TARGET ${TargetName} APPEND_STRING PROPERTY LINK_FLAGS " -flto -fuse-ld=gold -fprofile-instr-use=${ProfilePath}") | ||
set_property(TARGET ${TargetName} APPEND_STRING PROPERTY LINK_FLAGS " -flto -fprofile-instr-use=${ProfilePath}") | ||
if(NOT LD_LLVM) | ||
set_property(TARGET ${TargetName} APPEND_STRING PROPERTY LINK_FLAGS " -fuse-ld=gold") | ||
endif() | ||
else(HAVE_LTO) | ||
message(WARNING "LTO is not supported, skipping profile guided optimizations") | ||
endif(HAVE_LTO) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -855,7 +855,6 @@ elseif(CLR_CMAKE_TARGET_ARCH_ARM64) | |
set(VM_SOURCES_DAC_AND_WKS_ARCH | ||
${ARCH_SOURCES_DIR}/stubs.cpp | ||
exceptionhandling.cpp | ||
gcinfodecoder.cpp | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this change intentionally part of this PR? It seems unrelated at a glance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, there were some multiply defined symbols due to double inclusion of this file and LLD complained. |
||
) | ||
|
||
set(VM_HEADERS_DAC_AND_WKS_ARCH | ||
|
@@ -865,6 +864,7 @@ elseif(CLR_CMAKE_TARGET_ARCH_ARM64) | |
|
||
set(VM_SOURCES_WKS_ARCH | ||
${ARCH_SOURCES_DIR}/profiler.cpp | ||
gcinfodecoder.cpp | ||
) | ||
|
||
if(CLR_CMAKE_HOST_UNIX) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will likely cause a merge conflict when the next time arcade folder is mirrored https://github.com/dotnet/arcade/blob/dde80826e2fe464ef34182d2f786a6a51121d9a6/eng/common/templates/jobs/source-build.yml#L17.
There was a proposal to send a heads-up comment from bot account on PR that is modifying eng/common and the author is not dotnet-bot. I think it can save the author the hassle of undoing-and-redoing-the-right-way (which has happened a few times in runtime, aspnetcore and other repos).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thank you for noticing this, I have not noticed this was in the common folder. I'll make the same change in arcade, that should fix the future issue, I believe (I did something like that in the past).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, reading the comment in the file above the change, we can just revert the change in it as it is used for managed stuff only and lld is not needed in there. I've updated all references to the centos image I've found and haven't noticed the purpose.