[PATCH] D42267: [ThinLTO] Allow 0 to be a valid value for pruning interval for C LTO API.

[Steven Wu via Phabricator via llvm-commits]

steven_wu added a comment.

In https://reviews.llvm.org/D42267#999711, @kromanova wrote:

> In https://reviews.llvm.org/D42267#999581, @steven_wu wrote:
>
> > This indeed affects how ld64 interact with libLTO. If user didn't specify a value, ld64 will always call thinlto_codegen_set_cache_pruning_interval with interval 0. This is currently a no-op so it will use whatever the default value in libLTO. With this change, it will be force pruning.
> >
> > It is possible for us to make change to future ld64 to use different value but it is not going work very well for compatibility. If old ld64 loaded a new libLTO, it is going to lose incremental build.
>
>
>
>
>   Well, the main reason for this change was:
>
>
> (a) compatibility with C++ API. A lot of code in libLTO is common for C API and C++ API. Having the same semantics for the same options in two different APIs will make code simpler and prevents from someone who only cares about C+ API to unintentionally break the C API functionality. The latter is particularly important, since incremental linking is not thoroughly tested.
>
> (b) giving a possibility to a user to run a pruner immediately, vs. "no-op" which could get achieved simply by not passing "pruning-interval" option to the linker, making option value 0 useless.
>
> I understand your compatibility issue, but I don't think it's very serious.
>
> - I doubt it's common for a user to pass a option to the linker, if the same effect could be achieved by not passing any option at all (why would user do extra work?).
> - This option will cause the pruner to run, which not necessarily means that the cache files will get removed and incremental build won't happen.  Cache files will get removed only if at the same time the files are stale, or their sizes are larger then certain thresholds, which are passed as parameters.
> - The pruner runs just 20 minutes earlier than it would have run by default. So, only the builds that are done more frequent than 20 minutes might be affected.
> - Only the users who pick up the linker and libLTO from different packages will get affected.
>
>   Let me know what you decide. If you want to keep things the old way, I could modify this patch for PS4 only.
>
>   So, to summarize, we could add a meaningful value for pruning_interval option, which currently can't get achieved otherwise, and additionally we will offer compatibility with C++API. The only users that will be affected are the ones who because of whatever reason decided to explicitly specify a default option on the command line rather than skipping it, whose builds are happening more frequently than 20 minutes and who had mixed and matched different releases of ld64 and libLTO.


I am definitely not against the change. It would be good to have the number means the same thing in C/C++ API. I think ld64 can be updated in advance before we release 7.0 so we can have a larger compatibility window so I don't think this should be a blocker.

I don't really like that we have the default in multiple places depending on which tool you use. So it seems we have default pruning interval in CachePruningPolicy and llvm-lto (and maybe in some linker that uses the C/C++ interfaces as well). That is probably the reason why C API choose to ignore 0 because if you have a cl::opt initialize to zero, it means it will respect the default from CachePruningPolicy.


Repository:
  rL LLVM

https://reviews.llvm.org/D42267