This is an archive of the discontinued LLVM Phabricator instance.

[LTO] Remove options to disable inlining and GVNLoadPRE.
ClosedPublic

Authored by fhahn on Jan 15 2021, 7:49 AM.

Details

Summary

This patch removes some ancient options as a clean-up before moving
code-gen to use LTOBackend in D94487.

I think it would preferable to remove those ancient options, because

  1. There are no corresponding options in LTOBackend based tools,
  2. There are no unit tests for them,
  3. They are not passed through by Clang,
  4. At least for GNVLoadPRE, users could just use GVN's enable-load-pre.

Alternatively we could add support for those options to lto::Config &
co, but I think it would be better to remove them, unless they are
actually used in practice.

Diff Detail

Event Timeline

fhahn created this revision.Jan 15 2021, 7:49 AM
fhahn requested review of this revision.Jan 15 2021, 7:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 7:49 AM

Agree, these should just be removed. but I also don't see any uses of disable-lto-vectorization in a test, can that and the corresponding DisableVectorization parameter be nuked as well?

fhahn updated this revision to Diff 316996.Jan 15 2021, 9:22 AM

Agree, these should just be removed. but I also don't see any uses of disable-lto-vectorization in a test, can that and the corresponding DisableVectorization parameter be nuked as well?

Sounds good, let's try to get rid of those custom options. To disable vectorization, we already use a way to works better with LTO, by adding metadata to disable vectorization.

steven_wu accepted this revision.Jan 15 2021, 9:29 AM

Sounds like a good idea to me!

This revision is now accepted and ready to land.Jan 15 2021, 9:29 AM
tejohnson added inline comments.Jan 15 2021, 9:32 AM
llvm/lib/LTO/LTOCodeGenerator.cpp
579–581

Both ThinLTOCodeGenerator.cpp and LTOBackend.cpp set these to true unconditionally. Should we be consistent here?

fhahn added inline comments.Jan 15 2021, 9:42 AM
llvm/lib/LTO/LTOCodeGenerator.cpp
579–581

Ah right, I was originally looking at the code in the gold-plugin, which sets it based on the opt-level. But you are right, the option is ignored in LTOBackend.cpp when using the legacy pass manager. I'll update the code here to also set it unconditionally.

fhahn updated this revision to Diff 317005.Jan 15 2021, 9:44 AM

enable vectorization unconditionally, to be in line with the behavior in LTOBackend.cpp.

tejohnson accepted this revision.Jan 15 2021, 9:55 AM

lgtm, thanks!

This revision was landed with ongoing or failed builds.Jan 16 2021, 8:43 AM
This revision was automatically updated to reflect the committed changes.