Pass -mglobal-merge as a module flag metadata.

[ab]

Summary

Per title, we used to pass -mno-global-merge when specified, and enable GlobalMerge always (leaving it to LLVM to decide whether to actually enable it: it does, on ARM and AArch64, for -O1 and above).

This addresses two problems (I'm fine with splitting the patch if desired):

• with LTO, passing -mno-global-merge didn't do anything, since LTO implies -O3 from the backend's standpoint.
• GlobalMerge was enabled by -O1. Instead, on AArch64, enable it at -O3, or when -mglobal-merge is specified. (the goal is to have it predicated by -O3 on ARM as well, but I still need to measure before doing that.)

Now, -mglobal-merge and -mno-global-merge are both explicitly passed by the driver.
They are then passed to the backend using module flag metadata (bikeshedding time: name is prose, and value is string. We can change both, and AFAICT there's no similar backend flag to be consistent with, yet.)

Thanks!
-Ahmed

[ahatanak]

Just one question:

Should it be an error if two files compiled with -mglobal-merge and -mno-global-merge are linked? With this patch applied, the merged IR (foo3.ll) gets "Enable Global Merge"="true" in such case.

$ clang foo1.c -o foo1.ll -mglobal-merge ...
$ clang foo2.c -o foo2.ll -mno-global-merge ...
$ lvm-link foo1.ll foo2.ll -o foo3.ll

lib/CodeGen/CodeGenModule.cpp

3617

Do you need to pass LLVMContext here? I saw a build error when I applied this patch.

[ab]

Good point, both issues are caused by a broken last-minute change: I made the module flag to be emitted only if "true", because I didn't want "false" flags littered in every module emitted (even on targets where it doesn't make sense).

I'll just revert to always emitting the flag, whether true or false, but only on ARM/AArch64. Does that sound OK?

-Ahmed

[ab]

And yes, in case that wasn't clear, conflicting flags should be an error.

[ahatanak]

How about the following case:

$ clang foo1.c -S -arch arm64 -o foo1.ll -O3
$ clang foo2.c -S -arch arm64 -o foo2.ll -mno-global-merge

foo1.ll would get "Enable Global Merge"="true" while foo2.ll would get "Enable Global Merge"="false" and linking the two modules would be an error. Should the linker emit a linked module with "Enable Global Merge"="false" without any errors in this case?

My question is, if -mno-global-merge/-mglobal-merge is provided on the command line, does it mean global-merge pass should be disabled or enabled regardless of what the optimization level is?

[ab]

• Only emit flag when GlobalMerge is disabled (per Akira and Duncan's reviews)

I added a target check to only emit the flags on ARM/AArch64. I'm fine with emitting the flag no matter the target, if desired (less brittle), but I personally don't like adding noise to every module ever, hence hiding it.

[dexonsmith]

+echristo

[ab]

• Switch to a "Global Merge" flag, enabled by default.
• Generate i32 values instead of strings.
• Add X86 tests (to make sure we don't emit the flags there).

[ab]

+echristo as a reviewer, to chime in on the use of flags.

-Ahmed

[echristo]

Hrm. What do you think about requiring that flags like this for LTO are just required to be passed on the command line rather than left in the module and subject to module merge? If so, you can just pass the option to the backend via TargetOptions which will put it on the TargetMachine and make it easy to grab for the pass manager.

Thoughts?

-eric

[echristo]

No, you probably haven't. I was seeing it as clang doing to lto link of the module together and then codegen based on that (which means it would have the options), but...

That said, I think the general problem is more specific. I.e. how do you specify -msse3 as part of the default code generation flags when you do lto?

The C++ interface has addAttr (which is painful in that it requires, as you say, every linker to understand llvm's command line interface), but this is also pretty painful:

const void *compile(size_t *length,
                    bool disableOpt,
                    bool disableInline,
                    bool disableGVNLoadPRE,
                    bool disableVectorization,
                    std::string &errMsg);

because, you know, all optimizations, inlining, gvnloadpre, and vectorization are all anyone care about :)

Realize this has dovetailed into "let's solve the general problem" but I am curious. The gold plugin's methods aren't much better.

Or am I missing something?

-eric

include/clang/Driver/Options.td

 def mfix_and_continue : Flag<["-"], "mfix-and-continue">, Group<clang_ignored_m_Group>;
 def mieee_fp : Flag<["-"], "mieee-fp">, Group<clang_ignored_m_Group>;
 def minline_all_stringops : Flag<["-"], "minline-all-stringops">, Group<clang_ignored_m_Group>;
 def mno_inline_all_stringops : Flag<["-"], "mno-inline-all-stringops">, Group<clang_ignored_m_Group>;
 def mfloat_abi_EQ : Joined<["-"], "mfloat-abi=">, Group<m_Group>;
 def mfpmath_EQ : Joined<["-"], "mfpmath=">, Group<m_Group>;
 def mfpu_EQ : Joined<["-"], "mfpu=">, Group<m_Group>;
 def mhwdiv_EQ : Joined<["-"], "mhwdiv=">, Group<m_Group>;
-def mglobal_merge : Flag<["-"], "mglobal-merge">, Group<m_Group>;
+def mglobal_merge : Flag<["-"], "mglobal-merge">, Group<m_Group>, Flags<[CC1Option]>;
 def mhard_float : Flag<["-"], "mhard-float">, Group<m_Group>;
 def miphoneos_version_min_EQ : Joined<["-"], "miphoneos-version-min=">, Group<m_Group>;
 def mios_version_min_EQ : Joined<["-"], "mios-version-min=">,
   Alias<miphoneos_version_min_EQ>, HelpText<"Set iOS deployment target">;
 def mios_simulator_version_min_EQ : Joined<["-"], "mios-simulator-version-min=">, Alias<miphoneos_version_min_EQ>;
 def mkernel : Flag<["-"], "mkernel">, Group<m_Group>;
 def mlinker_version_EQ : Joined<["-"], "mlinker-version=">,
   Flags<[DriverOption]>;

lib/CodeGen/BackendUtil.cpp

TargetMachine *EmitAssemblyHelper::CreateTargetMachine(bool MustCreateTM) {
   if (!CodeGenOpts.LimitFloatPrecision.empty()) {
     BackendArgs.push_back("-limit-float-precision");
     BackendArgs.push_back(CodeGenOpts.LimitFloatPrecision.c_str());
   }
   if (llvm::TimePassesIsEnabled)
     BackendArgs.push_back("-time-passes");
   for (unsigned i = 0, e = CodeGenOpts.BackendOptions.size(); i != e; ++i)
     BackendArgs.push_back(CodeGenOpts.BackendOptions[i].c_str());
-  if (CodeGenOpts.NoGlobalMerge)
-    BackendArgs.push_back("-enable-global-merge=false");
   BackendArgs.push_back(nullptr);
   llvm::cl::ParseCommandLineOptions(BackendArgs.size() - 1,
                                     BackendArgs.data());
 
   std::string FeaturesStr;
   if (!TargetOpts.Features.empty()) {
     SubtargetFeatures Features;
     for (std::vector<std::string>::const_iterator

lib/CodeGen/CodeGenModule.h

private:
   void EmitDeclMetadata();
 
   /// \brief Emit the Clang version as llvm.ident metadata.
   void EmitVersionIdentMetadata();
 
   /// Emits target specific Metadata for global declarations.
   void EmitTargetMetadata();
 
+  /// Emit the module flag metadata used to pass options controlling the
+  /// the backend to LLVM.
+  void EmitBackendOptionsMetadata();
+
   /// Emit the llvm.gcov metadata used to tell LLVM where to emit the .gcno and
   /// .gcda files in a way that persists in .bc files.
   void EmitCoverageFile();
 
   /// Emits the initializer for a uuidof string.
   llvm::Constant *EmitUuidofInitializer(StringRef uuidstr);
 
   /// Determine whether the definition must be emitted; if this returns \c

lib/CodeGen/CodeGenModule.cpp

if (getCodeGenOpts().EmitGcovArcs || getCodeGenOpts().EmitGcovNotes)
     EmitCoverageFile();
 
   if (DebugInfo)
     DebugInfo->finalize();
 
   EmitVersionIdentMetadata();
 
   EmitTargetMetadata();
+
+  EmitBackendOptionsMetadata();
 }
 
 void CodeGenModule::UpdateCompletedType(const TagDecl *TD) {
   // Make sure that this type is translated.
   Types.UpdateCompletedType(TD);
 }
 
 llvm::MDNode *CodeGenModule::getTBAAInfo(QualType QTy) {
void CodeGenModule::EmitTargetMetadata() {
   for (unsigned I = 0; I != MangledDeclNames.size(); ++I) {
     auto Val = *(MangledDeclNames.begin() + I);
     const Decl *D = Val.first.getDecl()->getMostRecentDecl();
     llvm::GlobalValue *GV = GetGlobalValue(Val.second);
     getTargetCodeGenInfo().emitTargetMD(D, GV, *this);
   }
 }
 
+void CodeGenModule::EmitBackendOptionsMetadata() {
+  // Only emit the "Global Merge" flag on targets that support it (ARM/AArch64).
+  llvm::Triple::ArchType Arch = Context.getTargetInfo().getTriple().getArch();
+  if (Arch == llvm::Triple::arm || Arch == llvm::Triple::armeb ||
+       Arch == llvm::Triple::thumb || Arch == llvm::Triple::thumbeb ||
+       Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::aarch64_be) {
+    if (getCodeGenOpts().NoGlobalMerge)
+      getModule().addModuleFlag(llvm::Module::Override, "Global Merge", 0u);
+    else
+      getModule().addModuleFlag(llvm::Module::Error, "Global Merge", 1u);
+  }
+}
+
 void CodeGenModule::EmitCoverageFile() {
   if (!getCodeGenOpts().CoverageFile.empty()) {
     if (llvm::NamedMDNode *CUNode = TheModule.getNamedMetadata("llvm.dbg.cu")) {
       llvm::NamedMDNode *GCov = TheModule.getOrInsertNamedMetadata("llvm.gcov");
       llvm::LLVMContext &Ctx = TheModule.getContext();
       llvm::MDString *CoverageFile =
           llvm::MDString::get(Ctx, getCodeGenOpts().CoverageFile);
       for (int i = 0, e = CUNode->getNumOperands(); i != e; ++i) {

lib/Driver/Tools.cpp

if (Arg *A = Args.getLastArg(options::OPT_mno_unaligned_access,
       else {
         if (Triple.getSubArch() == llvm::Triple::SubArchType::ARMSubArch_v6m)
           D.Diag(diag::err_target_unsupported_unaligned) << "v6m";
         CmdArgs.push_back("-arm-no-strict-align");
       }
     }
   }
 
-  // Setting -mno-global-merge disables the codegen global merge pass. Setting 
-  // -mglobal-merge has no effect as the pass is enabled by default.
+  // Forward the -mglobal-merge option, for explicit control over the
+  // global-merge pass.
   if (Arg *A = Args.getLastArg(options::OPT_mglobal_merge,
                                options::OPT_mno_global_merge)) {
     if (A->getOption().matches(options::OPT_mno_global_merge))
       CmdArgs.push_back("-mno-global-merge");
+    else
+      CmdArgs.push_back("-mglobal-merge");
   }
 
   if (!Args.hasFlag(options::OPT_mimplicit_float,
                     options::OPT_mno_implicit_float,
                     true))
     CmdArgs.push_back("-no-implicit-float");
 
   // llvm does not support reserving registers in general. There is support
if (Arg *A = Args.getLastArg(options::OPT_mfix_cortex_a53_835769,
     else
       CmdArgs.push_back("-aarch64-fix-cortex-a53-835769=0");
   } else if (Triple.getEnvironment() == llvm::Triple::Android) {
     // Enabled A53 errata (835769) workaround by default on android
     CmdArgs.push_back("-backend-option");
     CmdArgs.push_back("-aarch64-fix-cortex-a53-835769=1");
   }
 
-  // Setting -mno-global-merge disables the codegen global merge pass. Setting
-  // -mglobal-merge has no effect as the pass is enabled by default.
+  // Forward the -mglobal-merge option, for explicit control over the
+  // global-merge pass.
   if (Arg *A = Args.getLastArg(options::OPT_mglobal_merge,
                                options::OPT_mno_global_merge)) {
     if (A->getOption().matches(options::OPT_mno_global_merge))
       CmdArgs.push_back("-mno-global-merge");
+    else
+      CmdArgs.push_back("-mglobal-merge");
   }
 
   if (Args.hasArg(options::OPT_ffixed_x18)) {
     CmdArgs.push_back("-backend-option");
     CmdArgs.push_back("-aarch64-reserve-x18");
   }
 }
 

lib/Frontend/CompilerInvocation.cpp

if (Arg *A = Args.getLastArg(OPT_mcode_model)) {
     if (Value == "small" || Value == "kernel" || Value == "medium" ||
         Value == "large")
       return Value;
     Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Value;
   }
   return "default";
 }
 
+static bool shouldEnableGlobalMergeForTarget(const llvm::Triple &T,
+                                             const unsigned OptLevel) {
+  llvm::Triple::ArchType Arch = T.getArch();
+  // FIXME: ARM should probably also require -O3.
+  return (OptLevel > 0 &&
+          (Arch == llvm::Triple::arm || Arch == llvm::Triple::armeb ||
+           Arch == llvm::Triple::thumb || Arch == llvm::Triple::thumbeb)) ||
+         (OptLevel > 2 &&
+          (Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::aarch64_be));
+}
+
 /// \brief Create a new Regex instance out of the string value in \p RpassArg.
 /// It returns a pointer to the newly generated Regex instance.
 static std::shared_ptr<llvm::Regex>
 GenerateOptimizationRemarkRegex(DiagnosticsEngine &Diags, ArgList &Args,
                                 Arg *RpassArg) {
   StringRef Val = RpassArg->getValue();
   std::string RegexError;
   std::shared_ptr<llvm::Regex> Pattern = std::make_shared<llvm::Regex>(Val);
static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.NoNaNsFPMath = (Args.hasArg(OPT_menable_no_nans) ||
                        Args.hasArg(OPT_cl_unsafe_math_optimizations) ||
                        Args.hasArg(OPT_cl_finite_math_only) ||
                        Args.hasArg(OPT_cl_fast_relaxed_math));
   Opts.NoSignedZeros = Args.hasArg(OPT_fno_signed_zeros);
   Opts.NoZeroInitializedInBSS = Args.hasArg(OPT_mno_zero_initialized_in_bss);
   Opts.BackendOptions = Args.getAllArgValues(OPT_backend_option);
   Opts.NumRegisterParameters = getLastArgIntValue(Args, OPT_mregparm, 0, Diags);
-  Opts.NoGlobalMerge = Args.hasArg(OPT_mno_global_merge);
+
+  Opts.NoGlobalMerge = !Args.hasFlag(
+      OPT_mglobal_merge, OPT_mno_global_merge,
+      shouldEnableGlobalMergeForTarget(llvm::Triple(TargetOpts.Triple),
+                                       Opts.OptimizationLevel));
+
   Opts.NoExecStack = Args.hasArg(OPT_mno_exec_stack);
   Opts.FatalWarnings = Args.hasArg(OPT_massembler_fatal_warnings);
   Opts.EnableSegmentedStacks = Args.hasArg(OPT_split_stacks);
   Opts.RelaxAll = Args.hasArg(OPT_mrelax_all);
   Opts.OmitLeafFramePointer = Args.hasArg(OPT_momit_leaf_frame_pointer);
   Opts.SaveTempLabels = Args.hasArg(OPT_msave_temp_labels);
   Opts.NoDwarfDirectoryAsm = Args.hasArg(OPT_fno_dwarf_directory_asm);
   Opts.SoftFloat = Args.hasArg(OPT_msoft_float);

test/CodeGen/global-merge.c

+// RUN: %clang_cc1 -emit-llvm -triple x86_64-- -mglobal-merge %s -o - | FileCheck %s -check-prefix=CHECK-NONE
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-- -mno-global-merge %s -o - | FileCheck %s -check-prefix=CHECK-NONE
+// RUN: %clang_cc1 -emit-llvm -triple x86_64-- -O3 %s -o - | FileCheck %s -check-prefix=CHECK-NONE
+
+// RUN: %clang_cc1 -emit-llvm -triple arm64-- -O0 %s -o - | FileCheck %s -check-prefix=CHECK-NGM
+// RUN: %clang_cc1 -emit-llvm -triple arm64-- -O1 %s -o - | FileCheck %s -check-prefix=CHECK-NGM
+// RUN: %clang_cc1 -emit-llvm -triple arm64-- -O3 %s -o - | FileCheck %s -check-prefix=CHECK-GM
+// RUN: %clang_cc1 -emit-llvm -triple arm64-- -O0 -mglobal-merge %s -o - | FileCheck %s -check-prefix=CHECK-GM
+// RUN: %clang_cc1 -emit-llvm -triple arm64-- -O3 -mno-global-merge %s -o - | FileCheck %s -check-prefix=CHECK-NGM
+
+// RUN: %clang_cc1 -emit-llvm -triple arm-- -O0 %s -o - | FileCheck %s -check-prefix=CHECK-NGM
+// RUN: %clang_cc1 -emit-llvm -triple arm-- -O1 %s -o - | FileCheck %s -check-prefix=CHECK-GM
+// RUN: %clang_cc1 -emit-llvm -triple arm-- -O3 %s -o - | FileCheck %s -check-prefix=CHECK-GM
+// RUN: %clang_cc1 -emit-llvm -triple arm-- -O0 -mglobal-merge %s -o - | FileCheck %s -check-prefix=CHECK-GM
+// RUN: %clang_cc1 -emit-llvm -triple arm-- -O3 -mno-global-merge %s -o - | FileCheck %s -check-prefix=CHECK-NGM
+
+// CHECK-NONE-NOT: Global Merge
+
+// CHECK-GM: !llvm.module.flags = !{{{.*}}}
+// CHECK-GM: !{{[0-9]+}} = !{i32 1, !"Global Merge", i32 1}
+
+// CHECK-NGM: !llvm.module.flags = !{{{.*}}}
+// CHECK-NGM: !{{[0-9]+}} = !{i32 4, !"Global Merge", i32 0}

test/Driver/mglobal-merge.c

 // RUN: %clang -target armv7-apple-darwin10 \
 // RUN:   -mglobal-merge -### -fsyntax-only %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-GM < %t %s
 
 // RUN: %clang -target arm64-apple-ios7 \
 // RUN:   -mglobal-merge -### -fsyntax-only %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-GM < %t %s
 
-// CHECK-GM-NOT: "-mglobal-merge"
+// CHECK-GM: "-mglobal-merge"
 
+
+// RUN: %clang -target x86_64-apple-darwin10 \
+// RUN:   -mno-global-merge -### -fsyntax-only %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-NONE < %t %s
+
+// RUN: %clang -target x86_64-apple-darwin10 \
+// RUN:   -mglobal-merge -### -fsyntax-only %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-NONE < %t %s
+
+// CHECK-NONE-NOT: "-mglobal-merge"
+// CHECK-NONE-NOT: "-mno-global-merge"