Pass -ffunction-sections/-fdata-sections along to gold-plugin

[tejohnson]

Summary

These options need to be passed to the plugin in order to have
an effect on LTO/ThinLTO compiles.

[pcc]

Perhaps it would be better to enable -ffunction-sections/-fdata-sections unconditionally at the linker level if the linker supports it? See also PR22999.

[tejohnson]

In D24644#557774, @pcc wrote:

Perhaps it would be better to enable -ffunction-sections/-fdata-sections unconditionally at the linker level if the linker supports it? See also PR22999.

My understanding is that these are not typically the default because it makes the resulting object files larger and the link slower. OTOH, for ThinLTO we definitely benefit more from these optimizations (and from PR22999 looks like there is a compelling reason to want this for LTO as well).

However, when you say enable them "unconditionally at the linker level" what do you mean - these need to go LLVM via plugin options. Are you suggesting having them enabled unconditionally in both gold-plugin and gold? That will require changes to both llvm and binutils, and the latter will have effects for other compilers. What about -Wl,-gc-sections, isn't that also needed to make effective use of these options?

[pcc]

Are you suggesting having them enabled unconditionally in both gold-plugin and gold? That will require changes to both llvm and binutils, and the latter will have effects for other compilers.

I mean in the gold plugin only. There would not need to be any changes to gold because it does not take these parameters directly.

What about -Wl,-gc-sections, isn't that also needed to make effective use of these options?

Not necessarily, PR22999 would happen with or without gc-sections.

[echristo]

There are two things pushing and pulling here:

a) You want to be able to pass compiler code generation options at the time we're actually doing the code generation,
b) "Traditionally" we don't pass CFLAGS to the linker.

I think I'd like to see us passing more options down at code generation time and handling it explicitly. In particular for this we don't have the knowledge at link time of what was intended. Even if we only turn it on when we see -gc-sections we won't know if the programmer wants function and data sections or just the former. Or maybe they want function sections for some reason other than gc-sections.

In short, I'm more on the a) side here in what I want :)

To go to PR22999: I think it might be reasonable for the linker during code generation to turn on function/data-sections where it isn't reasonable for us to do so in the compiler. I can come up with weird cases to break it, but those are largely module level inline assembly.

[mehdi_amini]

What about function attributes? Hey that's the trend :)
You could have a subset of the functions in their own sections but not all. With LTO it means that you can disable this for a single input file.

[echristo]

In D24644#562286, @mehdi_amini wrote:

What about function attributes? Hey that's the trend :)
You could have a subset of the functions in their own sections but not all. With LTO it means that you can disable this for a single input file.

True, but we'd need data attributes too for -fdata-sections. That's the main reason I haven't migrated the options out of TargetOptions and into the IR here. Rough sketch on module merge time: We'd probably want to error on functions/data that had separate section set in one module but not in another - there are a few ways to make that not error at link time, but at that point you're really relying on weird linker side effects and it's probably not what you intended anyhow.

-eric

[tejohnson]

Ping. It seems like using attributes is not feasible at this time due to the lack of data attributes.

[mehdi_amini]

LGTM.

[fhahn]

I'd like to fix PR22999 and was wondering if you think adding a function-section attribute to the IR would be a viable solution?

When doing LTO, we could add the same function-section to each function in a module in the IRLinker. @mehdi_amini did you think something like that when suggesting using attributes?

ps: Maybe it would be better to discuss this at the bug report, but this review seemed the easiest way to address the right people.

[mehdi_amini]

In D24644#792168, @fhahn wrote:

I'd like to fix PR22999 and was wondering if you think adding a function-section attribute to the IR would be a viable solution?

When doing LTO, we could add the same function-section to each function in a module in the IRLinker. @mehdi_amini did you think something like that when suggesting using attributes?

Not sure how this would work? How do you codegen half of the functions with function-section but not all? How is the backend supposed to behave if it starts with a function that isn't decorated with this attribute, then move to one that is, and finally proceed with one that isn't?

[fhahn]

In D24644#792262, @mehdi_amini wrote:

In D24644#792168, @fhahn wrote:

I'd like to fix PR22999 and was wondering if you think adding a function-section attribute to the IR would be a viable solution?

When doing LTO, we could add the same function-section to each function in a module in the IRLinker. @mehdi_amini did you think something like that when suggesting using attributes?

Not sure how this would work? How do you codegen half of the functions with function-section but not all? How is the backend supposed to behave if it starts with a function that isn't decorated with this attribute, then move to one that is, and finally proceed with one that isn't?

hm yes I guess we would have to group the functions by their function-section attribute, which isn't such a good idea probably.

@echristo you mentioned that the compiler maybe could determine if we should use function-sections during codegen, but it does not seem possible to determine the size of the code section before emitting all functions, at which point it is already too late. Maybe I am missing something?

cfe/trunk/lib/Driver/Tools.cpp

static unsigned getLTOParallelism(const ArgList &Args, const Driver &D) {
   Arg *LtoJobsArg = Args.getLastArg(options::OPT_flto_jobs_EQ);
   if (LtoJobsArg &&
       StringRef(LtoJobsArg->getValue()).getAsInteger(10, Parallelism))
     D.Diag(diag::err_drv_invalid_int_value) << LtoJobsArg->getAsString(Args)
                                             << LtoJobsArg->getValue();
   return Parallelism;
 }
 
+// CloudABI and WebAssembly use -ffunction-sections and -fdata-sections by
+// default.
+static bool isUseSeparateSections(const llvm::Triple &Triple) {
+  return Triple.getOS() == llvm::Triple::CloudABI ||
+         Triple.getArch() == llvm::Triple::wasm32 ||
+         Triple.getArch() == llvm::Triple::wasm64;
+}
+
 static void AddGoldPlugin(const ToolChain &ToolChain, const ArgList &Args,
                           ArgStringList &CmdArgs, bool IsThinLTO,
                           const Driver &D) {
   // Tell the linker to load the plugin. This has to come before AddLinkerInputs
   // as gold requires -plugin to come before any -plugin-opt that -Wl might
   // forward.
   CmdArgs.push_back("-plugin");
   std::string Plugin =
if (Arg *A = Args.getLastArg(options::OPT_gTune_Group,
                                options::OPT_ggdbN_Group)) {
     if (A->getOption().matches(options::OPT_glldb))
       CmdArgs.push_back("-plugin-opt=-debugger-tune=lldb");
     else if (A->getOption().matches(options::OPT_gsce))
       CmdArgs.push_back("-plugin-opt=-debugger-tune=sce");
     else
       CmdArgs.push_back("-plugin-opt=-debugger-tune=gdb");
   }
+
+  bool UseSeparateSections =
+      isUseSeparateSections(ToolChain.getEffectiveTriple());
+
+  if (Args.hasFlag(options::OPT_ffunction_sections,
+                   options::OPT_fno_function_sections, UseSeparateSections)) {
+    CmdArgs.push_back("-plugin-opt=-function-sections");
+  }
+
+  if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,
+                   UseSeparateSections)) {
+    CmdArgs.push_back("-plugin-opt=-data-sections");
+  }
 }
 
 /// This is a helper function for validating the optional refinement step
 /// parameter in reciprocal argument strings. Return false if there is an error
 /// parsing the refinement step. Otherwise, return true and set the Position
 /// of the refinement step in the input string.
 static bool getRefinementStep(StringRef In, const Driver &D,
                               const Arg &A, size_t &Position) {
#endif
   }
 
   if (Args.hasFlag(options::OPT_fdebug_types_section,
                    options::OPT_fno_debug_types_section, false)) {
     CmdArgs.push_back("-backend-option");
     CmdArgs.push_back("-generate-type-units");
   }
 
-  // CloudABI and WebAssembly use -ffunction-sections and -fdata-sections by
-  // default.
-  bool UseSeparateSections = Triple.getOS() == llvm::Triple::CloudABI ||
-                             Triple.getArch() == llvm::Triple::wasm32 ||
-                             Triple.getArch() == llvm::Triple::wasm64;
+  bool UseSeparateSections = isUseSeparateSections(Triple);
 
   if (Args.hasFlag(options::OPT_ffunction_sections,
                    options::OPT_fno_function_sections, UseSeparateSections)) {
     CmdArgs.push_back("-ffunction-sections");
   }
 
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,
                    UseSeparateSections)) {

cfe/trunk/test/Driver/gold-lto-sections.c

+// RUN: touch %t.o
+//
+// RUN: %clang -target x86_64-unknown-linux -### %t.o -flto 2>&1 \
+// RUN:     -Wl,-plugin-opt=foo -O3 \
+// RUN:     -ffunction-sections -fdata-sections \
+// RUN:     | FileCheck %s
+// CHECK: "-plugin-opt=-function-sections"
+// CHECK: "-plugin-opt=-data-sections"