Add PGO support for Clang/LLVM on Unix #10533

dpodder · 2017-03-28T22:18:34Z

Extend PGO support from VC++ on WIN32 to Clang/LLVM on UNIX as well.

Just like on Windows: if profile data is missing, skip enabling PGO (allows non-PGO builds in branches where we don't publish PGO data).
PGO with LTO requires additional dependencies (namely a discoverable ld.gold and LLVMgold.so). To protect against broken support and keep the build flexible across a wider array of distros, attempt to detect whether PGO compilation would work (using cmake's try_compile()), and fall back to a non-PGO/non-LTO build if the test fails.

dpodder · 2017-03-28T22:19:43Z

ellismg · 2017-03-28T22:26:57Z

pgosupport.cmake

 endfunction(add_pgo)

+# Detect whether PGO is supported
+if(UNIX)


Other places in coreclr, we have configure.cmake which we use to do configure like tests. Can we do the same thing here? Using check_cxx_source_compiles should let you do this check without having to check in a new file.

Thanks! I wasn't aware of configure.cmake, but it definitely looks like the better approach. Will make the change.

I agree with @ellismg

ellismg · 2017-03-28T22:27:33Z

/cc @janvorli who may be a better reviewer.

janvorli · 2017-03-28T22:49:57Z

pgosupport.cmake

-        if(CLR_CMAKE_PGO_INSTRUMENT)
+    if(CLR_CMAKE_PGO_INSTRUMENT)
+        # Unfortunately LINK_FLAGS_* don't support generator expressions, so we need to use a loop
+        foreach(Config IN LISTS ConfigList)


It seems all this machinery of config lists and enumerations through it makes it a bit difficult to read. It seems that putting in just the two following lines for Win32 would be much simpler and would work the same way:

set_property(TARGET ${TargetName} APPEND_STRING PROPERTY LINK_FLAGS_RELEASE /LTCG /GENPROFILE)
set_property(TARGET ${TargetName} APPEND_STRING PROPERTY LINK_FLAGS_RELWITHDEBINFO /LTCG /GENPROFILE)

And for Unix, since makefile generator is not a multi-config generator, you can just use LINK_FLAGS with no build type added:
set_property(TARGET ${TargetName} APPEND_STRING PROPERTY LINK_FLAGS -flto -fuse-ld=gold -fprofile-instr-generate)

Similar for the CLR_CMAKE_PGO_INSTRUMENT case.

Sure, I'm happy to rewrite it without the loop and without the helper function to keep it easier to follow.

Problem with the suggestion for LINK_FLAGS is that even on Unix, we need the flags applied only when configuring for release or relwithdebinfo. If I add it to LINK_FLAGS, they would get applied even during a debug build. Generator expressions would be the go-to answer, but they're not supported for LINK_FLAGS or its variants.

I did some private testing and it seems like cmake does the right thing with LINK_FLAGS_{CONFIG}, even when targeting a single-config build system like make. The only other approach I can think of is to use target_link_libraries() to add linker flags instead--which do support generator expressions--but they end up putting the flags inside of the "additional libraries" msbuild property, which I didn't like the sight of (it works, but seems like more of a hack to me).

I am sorry. I don't know why I have not taken into account the fact that it should be set for optimized builds only for Unix too. So I'd also use the if (UPPERCASE_CMAKE_BUILD_TYPE STREQUAL RELEASE OR UPPERCASE_CMAKE_BUILD_TYPE STREQUAL RELWITHDEBINFO) for Unix like in the other place I've suggested or use double set_property the same way as on Windows, whichever you prefer.

janvorli · 2017-03-28T22:52:30Z

pgosupport.cmake

            if(WIN32)
-                set_property(TARGET ${TargetName} APPEND_STRING PROPERTY ${LinkFlagsProperty} "/LTCG /GENPROFILE")
+                append_prop_strings(${TargetName} LINK_FLAGS_${Config} /LTCG /GENPROFILE)
+            elseif(UNIX)


At all other places, we assume that WIN32 means !UNIX and vice versa and only use WIN32 as a condition. So it would be nice to keep it aligned with that.

Sounds good, I'll change it.

janvorli · 2017-03-28T22:56:25Z

pgosupport.cmake

+        foreach(Config IN LISTS ConfigList)
            if(WIN32)
-                set_property(TARGET ${TargetName} APPEND_STRING PROPERTY ${LinkFlagsProperty} "/LTCG /GENPROFILE")
+                append_prop_strings(${TargetName} LINK_FLAGS_${Config} /LTCG /GENPROFILE)


We already set the LTCG option in the root CMakeLists.txt

I can remove it, but I've already been setting it (on Windows) and it seems harmless to repeat it (PGO doesn't work without LTCG). Do we prefer for /LTCG to be centralized and a hard-fail if missed?

Hmm, I guess it is safer to keep it at both places then.

janvorli · 2017-03-28T23:09:28Z

pgosupport.cmake

+            endforeach(Config)
+            if(UNIX)
+                ## On Unix we need to pass PGO flags to the compiler as well as the linker
+                target_compile_options(${TargetName} PRIVATE $<${IsReleaseConfig}:-flto -fprofile-instr-use=${ProfilePath}>)


Since the makefile generator is not a multi-target one, you could just use if (UPPERCASE_CMAKE_BUILD_TYPE STREQUAL RELEASE OR UPPERCASE_CMAKE_BUILD_TYPE STREQUAL RELWITHDEBINFO), which we do in other places. Unless something leaked in without my knowledge, we use the generator expressions for WIN32 only, which uses a multi-target generator.

This seemed to work, at least on the version of cmake I was testing, but I'll double-check. Happy to change it if necessary, although if this does work down to the minimum version we officially support, I like the generator approach if only to keep it parallel with WIN32.

My point was that this is the only place that uses generator expressions for non WIN32, so it would be inconsistent with the rest of the stuff. I'd prefer keeping the convention used at other places.

Ah, okay. I'll make the change.

janvorli · 2017-03-28T23:10:03Z

pgosupport.cmake

 endfunction(add_pgo)

+# Detect whether PGO is supported
+if(UNIX)


I agree with @ellismg

* Move HAVE_PGO check into cmake.configure - Also renaming to HAVE_LTO to make intent more clear and reusable * Remove loop over release configs to make code easier to follow * Assume !WIN32 == UNIX (as is the case throughout the repo) * Use plain if condition instead of generator expression on UNIX

dpodder · 2017-03-29T18:58:01Z

@janvorli Thanks for the feedback, made the changes. PTAL

janvorli

LGTM, thank you!

Daniel Podder added 2 commits March 28, 2017 07:16

Add PGO support for Clang/LLVM on Unix

10dad0f

Autodetect PGO support when on Unix

d455300

dnfclas added the cla-already-signed label Mar 28, 2017

dpodder requested a review from janvorli March 28, 2017 22:18

ellismg reviewed Mar 28, 2017

View reviewed changes

janvorli reviewed Mar 28, 2017

View reviewed changes

janvorli approved these changes Mar 29, 2017

View reviewed changes

dpodder merged commit 926d104 into dotnet:master Mar 30, 2017

dpodder deleted the linuxpgo branch March 30, 2017 02:01

karelz modified the milestone: 2.0.0 Aug 28, 2017

jkotas mentioned this pull request Sep 14, 2017

implement __assume for non-windows builds #13891

Closed

Add PGO support for Clang/LLVM on Unix #10533

Add PGO support for Clang/LLVM on Unix #10533

Uh oh!

Conversation

dpodder commented Mar 28, 2017

Uh oh!

dpodder commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ellismg commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpodder commented Mar 29, 2017

Uh oh!

janvorli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants