This is an archive of the discontinued LLVM Phabricator instance.

LTO: Hash the parts of the LTO configuration that affect code generation.
ClosedPublic

Authored by pcc on Dec 7 2016, 5:57 PM.

Details

Summary

Most importantly, we need to hash the relocation model, otherwise we can
end up trying to link non-PIC object files into PIEs or DSOs.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 80700.Dec 7 2016, 5:57 PM
pcc retitled this revision from to LTO: Hash the parts of the LTO configuration that affect code generation..
pcc updated this object.
pcc added reviewers: tejohnson, mehdi_amini.
pcc added subscribers: llvm-commits, krasin.
tejohnson edited edge metadata.Dec 7 2016, 7:01 PM

Thanks for fixing this. Looks pretty good but a couple of questions.

llvm/include/llvm/CodeGen/CommandFlags.h
235 ↗(On Diff #80700)

Why is this new option being added here?

llvm/lib/LTO/LTO.cpp
85 ↗(On Diff #80700)

I don't understand the comment about all clients initializing Options from command line flags, but it being unsupported in production.

pcc added inline comments.Dec 7 2016, 7:40 PM
llvm/include/llvm/CodeGen/CommandFlags.h
235 ↗(On Diff #80700)

This is so that I can test the effect of turning on RelaxELFRelocations from llvm-lto2. There's currently no other way to do it.

This should probably have been added when the codegen flag was added. Somewhat surprisingly it looks like we are only testing this via clang (see clang/test/CodeGen/relax.c).

llvm/lib/LTO/LTO.cpp
85 ↗(On Diff #80700)

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

pcc updated this revision to Diff 80713.Dec 7 2016, 7:59 PM
pcc edited edge metadata.
  • Add FunctionSections, DataSections and DebuggerTuning; fix a bug
mehdi_amini accepted this revision.Dec 7 2016, 9:01 PM
mehdi_amini edited edge metadata.
mehdi_amini added inline comments.
llvm/include/llvm/CodeGen/CommandFlags.h
235 ↗(On Diff #80700)

Like Teresa, I'm not thrilled that there are many changes that are not totally related to the "bug fix" on a deficiency in the amount of things we are hashing.
However it may be overkill to split, so I'm fine with it as is.

llvm/lib/LTO/LTO.cpp
85 ↗(On Diff #80700)

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.

This revision is now accepted and ready to land.Dec 7 2016, 9:01 PM
pcc added inline comments.Dec 7 2016, 9:38 PM
llvm/lib/LTO/LTO.cpp
85 ↗(On Diff #80700)

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.

This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Dec 7 2016, 9:47 PM
llvm/lib/LTO/LTO.cpp
85 ↗(On Diff #80700)

+1 for the .def, I like the idea.

tejohnson added inline comments.Dec 7 2016, 11:26 PM
llvm/lib/LTO/LTO.cpp
85 ↗(On Diff #80700)

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?

pcc added inline comments.Dec 8 2016, 10:07 AM
llvm/lib/LTO/LTO.cpp
85 ↗(On Diff #80700)

Yes, this would be done via the .def file.