Page MenuHomePhabricator

[ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends
Needs ReviewPublic

Authored by tejohnson on Apr 2 2019, 4:05 PM.

Details

Reviewers
pcc
dexonsmith
Summary

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.

Event Timeline

tejohnson created this revision.Apr 2 2019, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2019, 4:05 PM

I wonder if we could add a module flag for the TLI, and then store that in the summary. Would it work for both implementations in that case?

I wonder if we could add a module flag for the TLI, and then store that in the summary. Would it work for both implementations in that case?

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.

I wonder if we could add a module flag for the TLI, and then store that in the summary. Would it work for both implementations in that case?

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:

  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:
  2. 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
  3. For the flag to disable all builtins, have a separate module flag with a Max merge (so we get the most conservative behavior)
  4. For the VectorLibrary, not sure - should it be an Error to merge modules with different libraries?
  5. 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)?

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).

tejohnson updated this revision to Diff 199320.Mon, May 13, 1:23 PM

Rework using module flags.

Herald added a project: Restricted Project. · View Herald TranscriptMon, May 13, 1:23 PM

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.

tejohnson retitled this revision from [ThinLTO] Support TargetLibraryInfoImpl in the backend to [ThinLTO] Add module flags for TargetLibraryInfoImpl and use in LTO backends.Tue, May 14, 6:38 AM
tejohnson edited the summary of this revision. (Show Details)
tejohnson added a reviewer: dexonsmith.
tejohnson added a subscriber: gchatelet.

Thanks for doing this. I think module flag is a good idea. Some comments inline.

clang/test/CodeGen/svml-calls.ll
16

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.

llvm/lib/LTO/LTOBackend.cpp
221

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?

tejohnson marked 2 inline comments as done.Tue, May 14, 1:33 PM
tejohnson added inline comments.
clang/test/CodeGen/svml-calls.ll
16

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.

llvm/lib/LTO/LTOBackend.cpp
221

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?

tejohnson marked an inline comment as done.Tue, May 14, 1:35 PM
tejohnson added inline comments.
llvm/lib/LTO/LTOBackend.cpp
221

I meant, "into the TargetLibraryInfoImpl constructor"

steven_wu added inline comments.Tue, May 14, 1:48 PM
llvm/lib/LTO/LTOBackend.cpp
221

SGTM

There is discussion of using function attributes to control this instead, see https://reviews.llvm.org/D61634.
I'll hold off on making the planned changes here until the overall approach is decided.