[ThinLTO]Allow setting of maximum cache size with 64-bit number, and provide C-interface function for large values
ClosedPublic

Authored by jhenderson on Thu, Sep 13, 4:03 AM.

Details

Summary

The maximum cache size in terms of bytes is a 64-bit number. However, the methods to set it only take unsigned, which means that the maximum cache size cannot be specified above 4GB. That's quite small compared to the output of some projects, so it makes sense to provide the ability to set larger values in that field.

We also need a C-interface function that provides a greater range than the existing thinlto_codegen_set_cache_size_bytes, which also only takes an unsigned, so this change also adds thinlto_codegen_set_cache_size_megabytes.

Diff Detail

Repository
rL LLVM
jhenderson created this revision.Thu, Sep 13, 4:03 AM
tejohnson added inline comments.Thu, Sep 13, 6:25 AM
include/llvm-c/lto.h
828 ↗(On Diff #165245)

Shouldn't this be changed to uint64_t? I don't think that would break any existing users since it is wider.

jhenderson added inline comments.Thu, Sep 13, 6:35 AM
include/llvm-c/lto.h
828 ↗(On Diff #165245)

That's what I originally thought (and I would prefer to do it if possible) but I think there are two issues, which may both be irrelevant:

  1. Is uint64_t definitely available in all versions of C that this header is intended to be compatible with? I don't know what the compatibility guarantees are.
  2. In the past, my team have occasionally updated the LTO library (as a DLL) without updating the header in our linker's source tree. If somebody were to do this in the future, (from one with the old, smaller size, to one with the newer size), I think this would result in unexpected behaviour (because the caller thinks it's calling an unsigned version, but the function called actually takes a uint64_t). In our case, that won't happen, since I'll be updating our local header once we integrate these changes in our downstream port, but I wasn't sure if this is something that might affect other users. I couldn't find any documentation explaining the compatibility guarantees of the C interface.
tejohnson added inline comments.Thu, Sep 13, 6:58 AM
include/llvm-c/lto.h
828 ↗(On Diff #165245)

Those are good questions and I don't know the answers as I don't have a lot of familiarity with the C interface. Hopefully someone else knows the compatibility guarantees there.

mehdi_amini added inline comments.Thu, Sep 13, 8:49 AM
include/llvm-c/lto.h
828 ↗(On Diff #165245)

IIRC: it is expected to preserve ABI, or when impossible, bump the version number (the linker is expected then to check it at runtime and adjust its behavior)

steven_wu accepted this revision.Thu, Sep 13, 10:01 AM

LGTM

include/llvm-c/lto.h
828 ↗(On Diff #165245)

I agree it needs to be ABI compatible and I can see changing the width of that value can cause problems, though I don't think change the cache size is going to break the build.

I would prefer the current approach, which is to add a new API. unsigned megabyte is not quite as large is uint64_t but I guess it is big enough.

This revision is now accepted and ready to land.Thu, Sep 13, 10:01 AM
jhenderson added inline comments.Fri, Sep 14, 2:39 AM
include/llvm-c/lto.h
828 ↗(On Diff #165245)

Thanks for the comments. I'm going to go ahead and land this as is.

I would prefer the current approach, which is to add a new API. unsigned megabyte is not quite as large is uint64_t but I guess it is big enough.

One alternative would be a function thinlto_codegen_set_cache_size_bytes_ull or similar, but I think this should be big enough for now.

jhenderson added inline comments.Fri, Sep 14, 5:46 AM
tools/llvm-lto/llvm-lto.cpp
161 ↗(On Diff #165245)

https://bugs.llvm.org/show_bug.cgi?id=19665 has meant this doesn't work on my Linux machine, so I'm changing it to unsigned long long.

This revision was automatically updated to reflect the committed changes.

I've had to back out rL342233, because the LLD cache test started failing intermittently, and I don't know why (it works for me locally). Would somebody mind trying to apply this patch and see if they get a failure?

I've had to back out rL342233, because the LLD cache test started failing intermittently, and I don't know why (it works for me locally). Would somebody mind trying to apply this patch and see if they get a failure?

Looks like the test failure was just a flaky test, so I've relanded this as rL342366.