[PATCH] D60162: [ThinLTO] Support TargetLibraryInfoImpl in the backend

Mon May 13 13:27:04 PDT 2019

tejohnson added a comment.

In D60162#1498392 <https://reviews.llvm.org/D60162#1498392>, @tejohnson wrote:

> In D60162#1498288 <https://reviews.llvm.org/D60162#1498288>, @tejohnson wrote:
>
> > Working on this one again as it previously wasn't causing any issues but just now got exposed in multiple places and started causing correctness issues. Two questions:
> >
> > 1. What should be the behavior when merging modules e.g. for LTO. I'm thinking of the following, with multiple module flags to specify different aspects: a) For the NoBuiltinFuncs I'm thinking of a module flag containing the list of strings, with AppendUnique merging behavior, so we get a superset in the merged module b) For the flag to disable all builtins, have a separate module flag with a Max merge (so we get the most conservative behavior) c) For the VectorLibrary, not sure - should it be an Error to merge modules with different libraries?
> > 2. While we get the above hammered out, would it be ok to submit this one (and companion clang change) to fix the bug (then remove when the module flags are added)?
>
>
> Weird, phabricator added numbering to all of my bullets, which formatted very differently than intended. Modified the above a bit to hopefully format better (2 high level questions, and some subquestions on the first one).

I went ahead and implemented the above approach, PTAL. I added test cases for ensuring that the various clang driver flags set up the module flags properly, that merging behaves as expected, and that these options correctly affect LTO backends.

Note that this patch now includes both clang and llvm changes. I am going to mark the old clang child patch as obsolete.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60162/new/

https://reviews.llvm.org/D60162