[LTO] Set CGOptLevel in LTO config.

[sbc100]

Summary

Previously we were never setting this which means it was always being
set to Default (-O2/-Os).

[sbc100]

Perhaps this was intentional? But heejin noticed that --lto-O0 builds were still runing at O2 GC. This code is that same as used in gold/gold-plugin.cpp.

[ruiu]

Is there any reason you don't want to set it to Aggressive by default? (e.g. hurts debuggability, makes code generation slow, etc.)

[sbc100]

From looking at other code it looks like Aggressive corresponds to -O3, and that --lto-O3 here.

[sbc100]

This behaviour seems to match what clang does: https://github.com/llvm-mirror/clang/blob/f7d44292db27680486c0f03fd8eba125276144c8/lib/CodeGen/BackendUtil.cpp#L369

[pcc]

I'd prefer that we not do this. The code gen level really ought be controlled by the frontend via function attributes. We shouldn't be setting it in gold-plugin.cpp either but the last time I tried to remove it it ended up getting reverted for causing a perf regression.

[sbc100]

I see. So there should be enough information in each of the bitcode files to set the CGOptLevel accordingly for each function?

The fact that gold saw a performance regression is suspicious no? Does that mean there is performance gain we can get by adding it here?

Perhaps we should remove the CGOptLevel from the LTO completely once we remove it from gold? It looks like tools/llvm-lto2/llvm-lto2.cpp also does that same thing BTW.

[pcc]

In D57422#1376482, @sbc100 wrote:

I see. So there should be enough information in each of the bitcode files to set the CGOptLevel accordingly for each function?

Not yet. It would need to be added and the cg passes taught to respect it.

The fact that gold saw a performance regression is suspicious no? Does that mean there is performance gain we can get by adding it here?

The user who saw the perf regression was using --lto-O3 IIRC. One of the proposed interim solutions was to use CGOptLevel = 2 for --lto-O[0-2] and CGOptLevel = 3 for --lto-O3, but I never got around to writing a patch for it.

Perhaps we should remove the CGOptLevel from the LTO completely once we remove it from gold? It looks like tools/llvm-lto2/llvm-lto2.cpp also does that same thing BTW.

Yes, that's the eventual plan.

[sbc100]

BTW, it looks like -O0 gets added to the bitcode as optnone, but O2 and O3 produce identical bitcode.

In D57422#1376492, @pcc wrote:

In D57422#1376482, @sbc100 wrote:

I see. So there should be enough information in each of the bitcode files to set the CGOptLevel accordingly for each function?

Not yet. It would need to be added and the cg passes taught to respect it.

In that case would you envisage even clang eventually not setting CGOptLevel globally and let each function set it own level?

The fact that gold saw a performance regression is suspicious no? Does that mean there is performance gain we can get by adding it here?

The user who saw the perf regression was using --lto-O3 IIRC. One of the proposed interim solutions was to use CGOptLevel = 2 for --lto-O[0-2] and CGOptLevel = 3 for --lto-O3, but I never got around to writing a patch for it.

Perhaps we should remove the CGOptLevel from the LTO completely once we remove it from gold? It looks like tools/llvm-lto2/llvm-lto2.cpp also does that same thing BTW.

Yes, that's the eventual plan.

In that case would you mind if I landed this change for now with a TODO to remove this code once the opt level for each function is plumbed through from the frontend? (Today it looks like only O0 is plumbed through), It seems that being consistent with the other LTO tools would be good for the time being, and there would be an immediate benefit for users who are passing --lto-O3.

[pcc]

In D57422#1376526, @sbc100 wrote:

BTW, it looks like -O0 gets added to the bitcode as optnone, but O2 and O3 produce identical bitcode.

In D57422#1376492, @pcc wrote:

In D57422#1376482, @sbc100 wrote:

I see. So there should be enough information in each of the bitcode files to set the CGOptLevel accordingly for each function?

Not yet. It would need to be added and the cg passes taught to respect it.

In that case would you envisage even clang eventually not setting CGOptLevel globally and let each function set it own level?

Yes.

The fact that gold saw a performance regression is suspicious no? Does that mean there is performance gain we can get by adding it here?

The user who saw the perf regression was using --lto-O3 IIRC. One of the proposed interim solutions was to use CGOptLevel = 2 for --lto-O[0-2] and CGOptLevel = 3 for --lto-O3, but I never got around to writing a patch for it.

Perhaps we should remove the CGOptLevel from the LTO completely once we remove it from gold? It looks like tools/llvm-lto2/llvm-lto2.cpp also does that same thing BTW.

Yes, that's the eventual plan.

In that case would you mind if I landed this change for now with a TODO to remove this code once the opt level for each function is plumbed through from the frontend? (Today it looks like only O0 is plumbed through), It seems that being consistent with the other LTO tools would be good for the time being, and there would be an immediate benefit for users who are passing --lto-O3.

This should be quite easy to do:

use CGOptLevel = 2 for --lto-O[0-2] and CGOptLevel = 3 for --lto-O3

I'd be happy if we changed gold and all the lld ports to do that.

[sbc100]

• Use Default CGOptLevel for --lto-[0..2]

[sbc100]

OK I updated this PR to use Default for 0-2 and Aggressive for 3.

I will followup with matching change to the other LTO using tools in llvm.

Can I ask why you don't want --lto-O0 to set CGOptLevel to None?

[pcc]

In D57422#1376550, @sbc100 wrote:

Can I ask why you don't want --lto-O0 to set CGOptLevel to None?

The brief answer is: --lto-O0 means no cross-module optimizations, and just because you don't want cross-module optimizations doesn't mean that you also want poor code quality. For example, control flow integrity requires LTO, and users of CFI who don't want cross-module optimizations (whether for code size, link performance or other reasons) should not expect the code quality to get worse just because they enabled LTO by necessity.

Common/Args.cpp

20

nit: CGOptLevel

29

Why not:

if (OptLevelLTO == 3)
  return CodeGenOpt::Aggressive;
assert(OptLevelLTO < 3);
return CodeGenOpt::Default;

?

[sbc100]

• feedback

[pcc]

LGTM

[mehdi_amini]

In D57422#1376600, @pcc wrote:

In D57422#1376550, @sbc100 wrote:

Can I ask why you don't want --lto-O0 to set CGOptLevel to None?

The brief answer is: --lto-O0 means no cross-module optimizations,

This is a poor convention IMO, because it conflict with the clang user exposed O0 which is for no optimizations.

and just because you don't want cross-module optimizations doesn't mean that you also want poor code quality.

In such case you shouldn't request something that is "O0"?

For example, control flow integrity requires LTO, and users of CFI who don't want cross-module optimizations (whether for code size, link performance or other reasons) should not expect the code quality to get worse just because they enabled LTO by necessity.

Why not exposing another option than "O0"? Coupling CFI to optimization even when O0 is requested does not seem in line with the usual practice: CFI is about behavior first.

COFF/LTO.cpp

 //===- LTO.cpp ------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "LTO.h"
 #include "Config.h"
 #include "InputFiles.h"
 #include "Symbols.h"
+#include "lld/Common/Args.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Strings.h"
 #include "lld/Common/TargetOptionsCommandFlags.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/IR/DiagnosticPrinter.h"
if (Config->Machine == COFF::IMAGE_FILE_MACHINE_I386)
     C.RelocModel = Reloc::Static;
   else
     C.RelocModel = Reloc::PIC_;
   C.DisableVerify = true;
   C.DiagHandler = diagnosticHandler;
   C.OptLevel = Config->LTOO;
   C.CPU = GetCPUStr();
   C.MAttrs = GetMAttrs();
+  C.CGOptLevel = args::getCGOptLevel(Config->LTOO);
 
   if (Config->SaveTemps)
     checkError(C.addSaveTemps(std::string(Config->OutputFile) + ".",
                               /*UseInputModulePath*/ true));
   lto::ThinBackend Backend;
   if (Config->ThinLTOJobs != 0)
     Backend = lto::createInProcessThinBackend(Config->ThinLTOJobs);
   return llvm::make_unique<lto::LTO>(std::move(C), Backend,

Common/Args.cpp

 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/Path.h"
 
 using namespace llvm;
 using namespace lld;
 
+// TODO(sbc): Remove this once CGOptLevel can be set completely based on bitcode
+// function metadata.
+CodeGenOpt::Level lld::args::getCGOptLevel(int OptLevelLTO) {
+  if (OptLevelLTO == 3)
+    return CodeGenOpt::Aggressive;
+  assert(OptLevelLTO < 3);
+  return CodeGenOpt::Default;
+}
+
 int lld::args::getInteger(opt::InputArgList &Args, unsigned Key, int Default) {
   auto *A = Args.getLastArg(Key);
   if (!A)
     return Default;
 
   int V;
   if (to_integer(A->getValue(), V, 10))
     return V;
 

ELF/LTO.cpp

 //===- LTO.cpp ------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "LTO.h"
 #include "Config.h"
 #include "InputFiles.h"
 #include "LinkerScript.h"
 #include "SymbolTable.h"
 #include "Symbols.h"
+#include "lld/Common/Args.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/TargetOptionsCommandFlags.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/Bitcode/BitcodeReader.h"
else
     C.RelocModel = Reloc::Static;
 
   C.CodeModel = GetCodeModelFromCMModel();
   C.DisableVerify = Config->DisableVerify;
   C.DiagHandler = diagnosticHandler;
   C.OptLevel = Config->LTOO;
   C.CPU = GetCPUStr();
   C.MAttrs = GetMAttrs();
+  C.CGOptLevel = args::getCGOptLevel(Config->LTOO);
 
   // Set up a custom pipeline if we've been asked to.
   C.OptPipeline = Config->LTONewPmPasses;
   C.AAPipeline = Config->LTOAAPipeline;
 
   // Set up optimization remarks if we've been asked to.
   C.RemarksFilename = Config->OptRemarksFilename;
   C.RemarksWithHotness = Config->OptRemarksWithHotness;

include/lld/Common/Args.h

 //===- Args.h ---------------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLD_ARGS_H
 #define LLD_ARGS_H
 
 #include "lld/Common/LLVM.h"
+#include "llvm/Support/CodeGen.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include <vector>
 
 namespace llvm {
 namespace opt {
 class InputArgList;
 }
 } // namespace llvm
 
 namespace lld {
 namespace args {
+
+llvm::CodeGenOpt::Level getCGOptLevel(int OptLevelLTO);
+
 int getInteger(llvm::opt::InputArgList &Args, unsigned Key, int Default);
+
 std::vector<StringRef> getStrings(llvm::opt::InputArgList &Args, int Id);
 
 uint64_t getZOptionValue(llvm::opt::InputArgList &Args, int Id, StringRef Key,
                          uint64_t Default);
 
 std::vector<StringRef> getLines(MemoryBufferRef MB);
 
 StringRef getFilenameWithoutExe(StringRef Path);
 
 } // namespace args
 } // namespace lld
 
 #endif

wasm/LTO.cpp

 //===- LTO.cpp ------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "LTO.h"
 #include "Config.h"
 #include "InputFiles.h"
 #include "Symbols.h"
+#include "lld/Common/Args.h"
 #include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Strings.h"
 #include "lld/Common/TargetOptionsCommandFlags.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/IR/DiagnosticPrinter.h"
static std::unique_ptr<lto::LTO> createLTO() {
 
   // Wasm currently only supports ThreadModel::Single
   C.Options.ThreadModel = ThreadModel::Single;
 
   C.DisableVerify = Config->DisableVerify;
   C.DiagHandler = diagnosticHandler;
   C.OptLevel = Config->LTOO;
   C.MAttrs = GetMAttrs();
+  C.CGOptLevel = args::getCGOptLevel(Config->LTOO);
 
   if (Config->Relocatable)
     C.RelocModel = None;
   else if (Config->Pic)
     C.RelocModel = Reloc::PIC_;
   else
     C.RelocModel = Reloc::Static;