[PATCH] D27556: LTO: Hash the parts of the LTO configuration that affect code generation.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/LTO/LTO.cpp:85
+  // FIXME: Hash more of Options. For now all clients initialize Options from
+  // command-line flags (which is unsupported in production), but may set
+  // RelaxELFRelocations.
----------------
mehdi_amini wrote:
> pcc wrote:
> > mehdi_amini wrote:
> > > pcc wrote:
> > > > tejohnson wrote:
> > > > > I don't understand the comment about all clients initializing Options from command line flags, but it being unsupported in production.
> > > > Both gold and lld read code generation options using `InitTargetOptionsFromCodeGenFlags` (see [0] for gold and [1] for lld), which reads command line flags passed via `-plugin-opt` (for gold) or `-mllvm` (for lld). I don't think we officially support either of those flags though; they should be considered more like debugging aids or at best like `clang -cc1` flags in that they should only be used by the toolchain to communicate with itself. So we don't actually need to hash any of those flags except for those being used by a client (either directly or indirectly by e.g. the clang driver).
> > > > 
> > > > Actually didn't you add support for passing through `-function-sections` and `-data-sections`? I guess we'll need to hash that as well. And someone else added `-debugger-tune` [2], so I guess we can add that to the pile.
> > > > 
> > > > [0] http://llvm-cs.pcc.me.uk/tools/llvm-lto/llvm-lto.cpp#749
> > > > [1] http://llvm-cs.pcc.me.uk/tools/lld/ELF/LTO.cpp#72
> > > > [2] http://llvm-cs.pcc.me.uk/?q=%22-plugin-opt
> > > I agree with pcc on the fact that we shouldn't use cl::opt as part of the LTO API.
> > > 
> > > For the hashing, I feel it'll be more maintainable to have the hash of the config generated by a method on the config itself (could take a hasher as an argument) so that we can better spot changes to the config that require changing the hash.
> > > 
> > > It would also be possible to hash the config *once* for the link and have each backend hash only the relevant part for this backend.
> > > For the hashing, I feel it'll be more maintainable to have the hash of the config generated by a method on the config itself (could take a hasher as an argument) so that we can better spot changes to the config that require changing the hash.
> > 
> > Maybe. For now I've added a comment at the top of Config.
> > 
> > For TargetOptions I was thinking of creating a TargetOptions.def file that would be used to define the fields in TargetOptions and could also be used to define the hash functions.
> +1 for the .def, I like the idea.
Good catch on the function-sections etc - why not add all Options fields to the hash? Or is that what you are essentially intending to do via the proposed .def file?


Repository:
  rL LLVM

https://reviews.llvm.org/D27556