add "Fast" option to LinkTimeOptimization API which enables LTCG:incremental for vs2010, and behaves as "On" for other toolsets#2549
Conversation
samsinsane
left a comment
There was a problem hiding this comment.
Default is supposed to do nothing so that you get the default of the tool you're using. The linked PR didn't change that behaviour, so I'm inclined to reject this PR.
|
When Premake specifies a "default" value, we are using the toolset default (i.e. if you used cl.exe, what is the behavior), not the default generated project from VS. I agree with @samsinsane and would agree that the behavior as implemented is the correct behavior. |
|
I see - the cl.exe default is WholeProgramOptimization being off. Would it be acceptable to create another PR that adds an "incremental" or "fast" option? The API changed the setting from incremental/fast to "full" when enabling it. |
|
Would this be a new API for Link Time Code Generation? (Fast, Full, etc) |
|
I think? I'm not well versed in the internal terms of premake :) Currently you can do I would add either or and keep the other as "on". The previous premake behaviour with was essentially "fast", with The behaviour now with "on" is So my question is - should "on" revert the behaviour back to the previous |
|
I would argue that we should have: In GCC and Clang, "On" should stay mapped to |
The API didn't introduce new behaviour for "on", it's exactly as it was with the |
|
Ah I see. The version I have been using was so old, it didn't have this change yet: This is what changed the default on behaviour to full from fast by adding the missing xml element. Ok, I will add a new "fast" option then. I will rename and force push this PR, so don't close it yet :) |
b1eff90 to
6189c3d
Compare
|
I've force pushed and renamed the PR. I just searched for "linktimeoptimization" and hopefully fixed up all the required places, and added tests. |
6189c3d to
36b7608
Compare
nickclark2016
left a comment
There was a problem hiding this comment.
See prior comments. Hit approve on accident.
417dcac to
4fd78b8
Compare
nickclark2016
left a comment
There was a problem hiding this comment.
Minor changes, but overall happy with the code. Suggest merging in master, since there's a decent amount of changes that landed in the last 48 hours.
tests/tools/test_clang.lua
Outdated
| test.isequal("windres", clang.gettoolname(cfg, "rc")) | ||
| end | ||
|
|
||
| function suite.tools_onLinkTimeOptimizationViaFlag() |
There was a problem hiding this comment.
The LTO flag was actually removed. The "via flag" test is no longer required. (Same comment on Clang, GCC, and MSC).
There was a problem hiding this comment.
Ah - I see. it would have been nice to keep that as deprecated option so I don't need to use different versions of premake for git bisecting old project files, but oh well :D
There was a problem hiding this comment.
All flags are being deprecated and removed ahead of the 5.0 full release. We'll be rolling out a new release soon with them all deprecated in favor of a dedicated API for each.
website/docs/linktimeoptimization.md
Outdated
| Premake 5.0-beta4 and later No newline at end of file | ||
| Premake 5.0-beta4 and later | ||
|
|
||
| Fast is available from Premake 5.0-beta8 or later. Applies to Visual Studio 2010 and clang, will behave as On for other toolsets. No newline at end of file |
There was a problem hiding this comment.
Given it's slightly more complex than just "Exists starting here", leave the "Premake 5.0-beta4 and later", then add a column to the table called Notes (see flags.md)
There was a problem hiding this comment.
Done - or get rid of the "available from Premake 5.0-beta8 or later" ?
…emental for vs2010, ftlo=thin for clang and behaves as "On" for other toolsets add missing clang toolset tests for LinkTimeOptimization
4fd78b8 to
df234e9
Compare
Done. I think I just accidentally started a rewiew and deleted the pending comment, oh well. What a mess this interface is. |
|
Thanks! |
add "Fast" option to LinkTimeOptimization API which enables LTCG:incremental for vs2010, and behaves as "On" for other toolsets
What does this PR do?
^ see commit description
How does this PR change Premake's behavior?
This adds a "Fast" option to the LinkTimeOptimiation for vs2010 projects.
Anything else we should know?
Did you check all the boxes?
closes #XXXXin comment to auto-close issue when PR is merged)You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!