Adds information used when creating the TargetLibraryInfoImpl as module flags.
Use these module flags to create a TargetLibraryInfoImpl in LTO backends.
Supports handling of -fno-builtin* and -fveclib options passed to distributed
backend clang invocations.
A module flag summarizing the TLI should theoretically work (we wouldn't need to put anything in the ThinLTO summary, just needs to be in the IR for the backends). There is a lot of stuff in the TLI though, it would all need to be summarized.
Regarding a summary, I believe that TLI's only inputs (currently) are the target triple, the list of disabled functions (or the setting to disable all functions), and the kind of vector functions enabled.
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:
- 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:
- 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
- For the flag to disable all builtins, have a separate module flag with a Max merge (so we get the most conservative behavior)
- For the VectorLibrary, not sure - should it be an Error to merge modules with different libraries?
- 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.
Thanks for doing this. I think module flag is a good idea. Some comments inline.
Personally, I think codegen tests like this will be cleaner to keep in LLVM. Clang tests just test the IRGen of the module flag and LLVM tests check that those flags are respected and module flag merge is respected.
Should this be done not just for LTOBackend but for regular compilation as well? LegacyCodegenerator and llc can all be benefit from a matching TargetLibraryInfo?
Ok. I originally was doing it here to ensure that ThinLTO distributed backends (which use clang) are fixed. But now that the approach no longer involves passing down additional info via that path into LTO, I don't think we need to test this explicitly but rather just as an LLVM LTO test. Will move.
Yeah, probably. I think the best way to have this utilized everywhere is to move the below code into the TargetLibraryInfoImpl itself - by having it also take the Module as a parameter). Probably as a required parameter, to ensure it is used consistently. WDYT?