Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'unifiedlto' option to gold plugin #121336

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

eleviant
Copy link
Contributor

Option allows using full LTO when linking bitcode files compiled with unified LTO pipeline.

Option allows using full LTO when linking bitcode files compiled with
unified LTO pipeline.
@eleviant eleviant self-assigned this Dec 30, 2024
@llvmbot llvmbot added the LTO Link time optimization (regular/full LTO or ThinLTO) label Dec 30, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 30, 2024

@llvm/pr-subscribers-lto

Author: None (eleviant)

Changes

Option allows using full LTO when linking bitcode files compiled with unified LTO pipeline.


Full diff: https://github.com/llvm/llvm-project/pull/121336.diff

3 Files Affected:

  • (added) llvm/test/tools/gold/X86/Inputs/unified-lto-foo.ll (+26)
  • (added) llvm/test/tools/gold/X86/unified-lto.ll (+42)
  • (modified) llvm/tools/gold/gold-plugin.cpp (+11-1)
diff --git a/llvm/test/tools/gold/X86/Inputs/unified-lto-foo.ll b/llvm/test/tools/gold/X86/Inputs/unified-lto-foo.ll
new file mode 100644
index 00000000000000..cd6cb16cd332c4
--- /dev/null
+++ b/llvm/test/tools/gold/X86/Inputs/unified-lto-foo.ll
@@ -0,0 +1,26 @@
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+@_g = dso_local local_unnamed_addr global i32 0, align 4
+@llvm.compiler.used = appending global [1 x ptr] [ptr @_g], section "llvm.metadata"
+
+define dso_local i32 @foo(i32 noundef %0) #0 {
+  %2 = add nsw i32 %0, 42
+  store i32 %2, ptr @_g, align 4
+  ret i32 %2
+}
+
+attributes #0 = { noinline }
+
+
+!llvm.module.flags = !{!0, !1}
+
+!0 = !{i32 1, !"EnableSplitLTOUnit", i32 1}
+!1 = !{i32 1, !"UnifiedLTO", i32 1}
+
+^0 = module: (path: "unified-lto-foo.o", hash: (1995435377, 1643957463, 3737953841, 2630641917, 1043992145))
+^1 = gv: (name: "foo", summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), insts: 3, funcFlags: (readNone: 0, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0, noInline: 1, alwaysInline: 0, noUnwind: 1, mayThrow: 0, hasUnknownCall: 0, mustBeUnreachable: 0), refs: (writeonly ^2)))) ; guid = 6699318081062747564
+^2 = gv: (name: "_g", summaries: (variable: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), varFlags: (readonly: 1, writeonly: 1, constant: 0)))) ; guid = 9713702464056781075
+^3 = gv: (name: "llvm.compiler.used", summaries: (variable: (module: ^0, flags: (linkage: appending, visibility: default, notEligibleToImport: 0, live: 1, dsoLocal: 0, canAutoHide: 0), varFlags: (readonly: 0, writeonly: 0, constant: 0), refs: (^3)))) ; guid = 9610627770985738006
+^4 = flags: 520
+^5 = blockcount: 0
diff --git a/llvm/test/tools/gold/X86/unified-lto.ll b/llvm/test/tools/gold/X86/unified-lto.ll
new file mode 100644
index 00000000000000..cc3e39b4b3b47f
--- /dev/null
+++ b/llvm/test/tools/gold/X86/unified-lto.ll
@@ -0,0 +1,42 @@
+; Check that we can use full LTO with gold plugin when inputs
+; are compiled using unified LTO pipeline
+; RUN: llvm-as %s -o %t.bc
+; RUN: llvm-as %p/Inputs/unified-lto-foo.ll -o %t-foo.bc
+; RUN: %gold -plugin %llvmshlibdir/LLVMgold%shlibext \
+; RUN:    -m elf_x86_64 \
+; RUN:    -plugin-opt=unifiedlto \
+; RUN:    -plugin-opt=save-temps \
+; RUN:    -u main \
+; RUN:    %t.bc %t-foo.bc \
+; RUN:    -o %t-out
+; RUN: llvm-dis %t-out.0.5.precodegen.bc -o - | FileCheck %s
+
+; Constant propagation is not supported by thin LTO.
+; With full LTO we fold argument into constant 43
+; CHECK:       define dso_local noundef i32 @main()
+; CHECK-NEXT:    tail call fastcc void @foo()
+; CHECK-NEXT:    ret i32 43
+
+; CHECK:       define internal fastcc void @foo()
+; CHECK-NEXT:    store i32 43, ptr @_g, align 4
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+define dso_local i32 @main() {
+  %1 = tail call i32 @foo(i32 noundef 1)
+  ret i32 %1
+}
+
+declare i32 @foo(i32 noundef)
+
+!llvm.module.flags = !{!0, !1}
+
+!0 = !{i32 1, !"EnableSplitLTOUnit", i32 1}
+!1 = !{i32 1, !"UnifiedLTO", i32 1}
+
+^0 = module: (path: "unified-lto.o", hash: (2850108895, 1189778381, 479678006, 1191715608, 4095117687))
+^1 = gv: (name: "foo") ; guid = 6699318081062747564
+^2 = gv: (name: "main", summaries: (function: (module: ^0, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 1, canAutoHide: 0), insts: 2, funcFlags: (readNone: 0, readOnly: 0, noRecurse: 0, returnDoesNotAlias: 0, noInline: 0, alwaysInline: 0, noUnwind: 1, mayThrow: 0, hasUnknownCall: 0, mustBeUnreachable: 0), calls: ((callee: ^1, tail: 1))))) ; guid = 15822663052811949562
+^3 = flags: 520
+^4 = blockcount: 0
diff --git a/llvm/tools/gold/gold-plugin.cpp b/llvm/tools/gold/gold-plugin.cpp
index 6d0021c85f20fb..ac2e9d4252aa38 100644
--- a/llvm/tools/gold/gold-plugin.cpp
+++ b/llvm/tools/gold/gold-plugin.cpp
@@ -154,6 +154,8 @@ namespace options {
   static std::string extra_library_path;
   static std::string triple;
   static std::string mcpu;
+  // Tells plugin to use unified lto
+  static bool unifiedlto = false;
   // When the thinlto plugin option is specified, only read the function
   // the information from intermediate files and write a combined
   // global index for the ThinLTO backends.
@@ -248,6 +250,8 @@ namespace options {
       TheOutputType = OT_DISABLE;
     } else if (opt == "emit-asm") {
       TheOutputType = OT_ASM_ONLY;
+    } else if (opt == "unifiedlto") {
+      unifiedlto = true;
     } else if (opt == "thinlto") {
       thinlto = true;
     } else if (opt == "thinlto-index-only") {
@@ -893,6 +897,7 @@ static std::unique_ptr<LTO> createLTO(IndexWriteCallback OnIndexWrite,
   Conf.OptLevel = options::OptLevel;
   Conf.PTO.LoopVectorization = options::OptLevel > 1;
   Conf.PTO.SLPVectorization = options::OptLevel > 1;
+  Conf.PTO.UnifiedLTO = options::unifiedlto;
   Conf.AlwaysEmitRegularLTOObj = !options::obj_path.empty();
 
   if (options::thinlto_index_only) {
@@ -972,8 +977,13 @@ static std::unique_ptr<LTO> createLTO(IndexWriteCallback OnIndexWrite,
   Conf.TimeTraceEnabled = !options::time_trace_file.empty();
   Conf.TimeTraceGranularity = options::time_trace_granularity;
 
+  LTO::LTOKind ltoKind = LTO::LTOK_Default;
+  if (options::unifiedlto)
+    ltoKind =
+        options::thinlto ? LTO::LTOK_UnifiedThin : LTO::LTOK_UnifiedRegular;
   return std::make_unique<LTO>(std::move(Conf), Backend,
-                                options::ParallelCodeGenParallelismLevel);
+                               options::ParallelCodeGenParallelismLevel,
+                               ltoKind);
 }
 
 // Write empty files that may be expected by a distributed build

@teresajohnson teresajohnson requested a review from ormris December 30, 2024 16:38
@teresajohnson
Copy link
Contributor

@ormris can you take a look? In particular, I'm confused at how this gets enabled via lld. It appears there isn't a separate unified LTO option there, and the mode is set unconditionally to UnifiedThin or UnifiedRegular based on the -flto= option:

ctx.arg.ltoKind = LtoKind::Default;
if (auto *arg = args.getLastArg(OPT_lto)) {
StringRef s = arg->getValue();
if (s == "thin")
ctx.arg.ltoKind = LtoKind::UnifiedThin;
else if (s == "full")
ctx.arg.ltoKind = LtoKind::UnifiedRegular;
else if (s == "default")
ctx.arg.ltoKind = LtoKind::Default;
else
ErrAlways(ctx) << "unknown LTO mode: " << s;
}
.

But then it looks like in the LTO code we will give an error if modules weren't built with -funified-lto:

if ((LTOMode == LTOK_UnifiedRegular || LTOMode == LTOK_UnifiedThin) &&
!LTOInfo->UnifiedLTO)
return make_error<StringError>(
"unified LTO compilation must use "
"compatible bitcode modules (use -funified-lto)",
inconvertibleErrorCode());

I can't recall how this is this supposed to work?

@eleviant
Copy link
Contributor Author

@ormris can you take a look? In particular, I'm confused at how this gets enabled via lld. It appears there isn't a separate unified LTO option there, and the mode is set unconditionally to UnifiedThin or UnifiedRegular based on the -flto= option:

ctx.arg.ltoKind = LtoKind::Default;
if (auto *arg = args.getLastArg(OPT_lto)) {
StringRef s = arg->getValue();
if (s == "thin")
ctx.arg.ltoKind = LtoKind::UnifiedThin;
else if (s == "full")
ctx.arg.ltoKind = LtoKind::UnifiedRegular;
else if (s == "default")
ctx.arg.ltoKind = LtoKind::Default;
else
ErrAlways(ctx) << "unknown LTO mode: " << s;
}

.
But then it looks like in the LTO code we will give an error if modules weren't built with -funified-lto:

if ((LTOMode == LTOK_UnifiedRegular || LTOMode == LTOK_UnifiedThin) &&
!LTOInfo->UnifiedLTO)
return make_error<StringError>(
"unified LTO compilation must use "
"compatible bitcode modules (use -funified-lto)",
inconvertibleErrorCode());

I can't recall how this is this supposed to work?

I think this shouldn't work. The only reason we don't really observe the issue is that clang is not passing --lto option to the linker. So lld is also "thinlto only" with -funified-lto.

@@ -0,0 +1,42 @@
; Check that we can use full LTO with gold plugin when inputs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add some kind of check that unified LTO inputs works with ThinLTO as well?

@teresajohnson
Copy link
Contributor

@ormris can you take a look? In particular, I'm confused at how this gets enabled via lld. It appears there isn't a separate unified LTO option there, and the mode is set unconditionally to UnifiedThin or UnifiedRegular based on the -flto= option:

ctx.arg.ltoKind = LtoKind::Default;
if (auto *arg = args.getLastArg(OPT_lto)) {
StringRef s = arg->getValue();
if (s == "thin")
ctx.arg.ltoKind = LtoKind::UnifiedThin;
else if (s == "full")
ctx.arg.ltoKind = LtoKind::UnifiedRegular;
else if (s == "default")
ctx.arg.ltoKind = LtoKind::Default;
else
ErrAlways(ctx) << "unknown LTO mode: " << s;
}

.
But then it looks like in the LTO code we will give an error if modules weren't built with -funified-lto:

if ((LTOMode == LTOK_UnifiedRegular || LTOMode == LTOK_UnifiedThin) &&
!LTOInfo->UnifiedLTO)
return make_error<StringError>(
"unified LTO compilation must use "
"compatible bitcode modules (use -funified-lto)",
inconvertibleErrorCode());

I can't recall how this is this supposed to work?

I think this shouldn't work. The only reason we don't really observe the issue is that clang is not passing --lto option to the linker. So lld is also "thinlto only" with -funified-lto.

Ok, I see. The lld --lto= option was added for Unified LTO (in https://reviews.llvm.org/D123805). This is a bit confusing IMO - @ormris can the name in lld be changed to --unifiedlto= to be clearer?

@eleviant
Copy link
Contributor Author

Addressed review comments

Copy link
Contributor

@teresajohnson teresajohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LTO Link time optimization (regular/full LTO or ThinLTO)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants