HomePhabricator

[Support][ThinLTO] Move ThinLTO caching to LLVM Support library

Authored by noajshu on Oct 18 2021, 6:40 PM.

Description

[Support][ThinLTO] Move ThinLTO caching to LLVM Support library

We would like to move ThinLTO’s battle-tested file caching mechanism to
the LLVM Support library so that we can use it elsewhere in LLVM.

Patch By: noajshu

Differential Revision: https://reviews.llvm.org/D111371

Event Timeline

This is really cool, thanks for generalizing this. That said, i'd love for you to continue to work on this to make it work more like the existing LLVM support libraries (e.g. not crashing the program on error).

/llvm/include/llvm/Support/Caching.h
1

What does "Configuration" mean here?

10

Please remove "This is used by ThinLTO." We don't generally talk about users of libraries in their implementations (since that will change/expand over time)

19

is Thread.h and MemoryBuffer needed here? I think you can forward declare the MemoryBuffer class.

23

Given that this is more generic than ThinLTO, I think you should remove the "Native Object" nomenclature. This could be used by other things that want to cache other sorts of files.

/llvm/lib/Support/Caching.cpp
9

Please describe what this does, now that it is more general than ThinLTO

17

Plz trim out the redundant #includes.

75

It may have been ok for ThinLTO to crash on errors like this, but more general libraries should return errors to the client, e.g. in an llvm::Error/ErrorOr or equivalent.

noajshu marked 6 inline comments as done.Nov 3 2021, 12:00 PM

This is really cool, thanks for generalizing this. That said, i'd love for you to continue to work on this to make it work more like the existing LLVM support libraries (e.g. not crashing the program on error).

Thanks for the comments! I have created D113080 which addresses most of your suggestions.