[2/3] TLS loads optimization (hoist)

[xiangzhangllvm]

Summary

We supported TLS loads optimization (LLVM part) at D120000

Now this patch mainly add Clang options (-mtls-load-hoist -mno-tls-load-hoist) for it.

Also we update the old llvm option to a bool option in this patch.

[efriedma]

clang/include/clang/Driver/Options.td

3627

clang driver options should generally have positive and negative versions (i.e. -mtls-load-hoist/-mno-tls-load-hoist).

I'm not sure I understand why we need to expose this as a clang option, though. In the initial patch, you mentioned you wanted to land it off-by-default initially... but I'm not sure why we can't just flip the internal switch to turn it on by default, and call it done.

[xiangzhangllvm]

clang/include/clang/Driver/Options.td

3627

First thanks for review!
Let me add "-mno-tls-load-hoist" , can I ask a question ? (sorry for I am not much familiar with the clang driver) Why clang driver should have both positive and negative options ? (if it default to negative, doesn't only add positive is enough?)

Yes, I land it off-by-default at [1/3] patch. Then I mentioned that:

"This is a general optimization which will affect all targets test, I'd like to enable it by another independent patch [3/3] with mainly modify tests."

Another purpose I want to just enable it in an independent patch is that I am not 100% sure it will has gain for all the targets. So I'd like to remain a easy way to revert the "enable patch" if necessary for some targets.

[efriedma]

clang/include/clang/Driver/Options.td

3627

In general, providing flags both ways has two benefits:

1. It lets users request a specific behavior even if the default changes in the future.
2. Sometimes, in complex build system, editing flags is tricky, so it's helpful to be able to override a flag by just sticking the inverse flag at the end of the command-line.

---

I think you're being too conservative in terms of the expected impact of this; it's hard for me to imagine a situation where hoisting a call to __tls_get_addr actually causes harm.

[xiangzhangllvm]

clang/include/clang/Driver/Options.td

3627

Thanks very much!

Caution is the parent of safety : )

[xiangzhangllvm]

Add -mno-tls-load-hoist option.

[MaskRay]

If the option is going to be on and never turned off, we don't need a clang driver option. Clang driver option has a commitment to relative stability.
For experimental things you can use cl::opt. cl::opt can be relatively freely removed.

[xiangzhangllvm]

In D122558#3415640, @MaskRay wrote:

If the option is going to be on and never turned off, we don't need a clang driver option. Clang driver option has a commitment to relative stability.
For experimental things you can use cl::opt. cl::opt can be relatively freely removed.

Make sense, I prefer to keep the clang driver option for customers
(in fact, this job is required from our customers and they point out they want an option to control it)
Yes, I added cl::opt for it, PLS refer TLSVariableHoist.cpp: line 43.
Thanks a lot for your reviewing!

[xiangzhangllvm]

Clang format

[xiangzhangllvm]

This patch is quite clear. Anyone can help accept it?

[MaskRay]

In D122558#3415767, @xiangzhangllvm wrote:

In D122558#3415640, @MaskRay wrote:

If the option is going to be on and never turned off, we don't need a clang driver option. Clang driver option has a commitment to relative stability.
For experimental things you can use cl::opt. cl::opt can be relatively freely removed.

Make sense, I prefer to keep the clang driver option for customers
(in fact, this job is required from our customers and they point out they want an option to control it)
Yes, I added cl::opt for it, PLS refer TLSVariableHoist.cpp: line 43.
Thanks a lot for your reviewing!

This doesn't look like sufficient justification adding a new driver option for a feature which should just be enabled at all time.
Seems like the option is to work around potential issues just in case?
You can do that with cl::opt. In case anything bad happens, you can either revert the patch or tell the customer to add -mllvm -xxx.
When the feature seems robust enough, remove the cl::opt option.
Anyway, I just think we should not commit to have such a driver option.

[xiangzhangllvm]

If the option is going to be on and never turned off

It is not supposed to be "never turned off".
Generally speaking, hoisting the common TLS address call operation will big probably have performance gains.
But this will also lengthen register interval, possible cause more spills.
especially in case

                   BB1
                 /      \
               BB2    BB3 (with 1 tls call or more tls calls but BB is cold)
(with 1 tls call)

And this patch only add about 15 lines to support the driver options. (Others are refining the llvm option and test)

[efriedma]

The problem isn't really the amount of code needed to support the option... it's that a formally documented option needs to have clear semantics, and we need to support it forever. We really don't like options that say "turn off optimization X that has no user-visible semantics". I mean, even if you pass -mno-tls-load-hoist, we might end up hoisting anyway, depending on what you're doing with the address...

[xiangzhangllvm]

In D122558#3420158, @efriedma wrote:

The problem isn't really the amount of code needed to support the option... it's that a formally documented option needs to have clear semantics, and we need to support it forever. We really don't like options that say "turn off optimization X that has no user-visible semantics". I mean, even if you pass -mno-tls-load-hoist, we might end up hoisting anyway, depending on what you're doing with the address...

Thank you! I begin more clear about the principle of Clang Driver option.
(It should prefer user-visible, functional, performance with visible control)
Let me abandon this patch and enable the optimization in default in PIC mode.

clang/include/clang/Basic/CodeGenOptions.def

CODEGENOPT(DebugPassManager, 1, 0) ///< Prints debug information for the new
                                    ///< pass manager.
 CODEGENOPT(DisableRedZone    , 1, 0) ///< Set when -mno-red-zone is enabled.
 CODEGENOPT(EmitCallSiteInfo, 1, 0) ///< Emit call site info only in the case of
                                    ///< '-g' + 'O>0' level.
 CODEGENOPT(EnableDIPreservationVerify, 1, 0) ///< Enable di preservation verify
                                              ///< each (it means check
                                              ///< the original debug info
                                              ///< metadata preservation).
+CODEGENOPT(TlsAddrLoadHoist, 1, 0) ///< Set when -mtls-load-hoist is specified.
 CODEGENOPT(IndirectTlsSegRefs, 1, 0) ///< Set when -mno-tls-direct-seg-refs
                                      ///< is specified.
 CODEGENOPT(DisableTailCalls  , 1, 0) ///< Do not emit tail calls.
 CODEGENOPT(NoEscapingBlockTailCalls, 1, 0) ///< Do not emit tail calls from
                                            ///< escaping blocks.
 CODEGENOPT(EmitDeclMetadata  , 1, 0) ///< Emit special metadata indicating what
                                      ///< Decl* various IR entities came from.
                                      ///< Only useful when running CodeGen as a

clang/include/clang/Driver/Options.td

 
 def mno_warn_nonportable_cfstrings : Flag<["-"], "mno-warn-nonportable-cfstrings">, Group<m_Group>;
 def mno_omit_leaf_frame_pointer : Flag<["-"], "mno-omit-leaf-frame-pointer">, Group<m_Group>;
 def momit_leaf_frame_pointer : Flag<["-"], "momit-leaf-frame-pointer">, Group<m_Group>,
   HelpText<"Omit frame pointer setup for leaf functions">;
 def moslib_EQ : Joined<["-"], "moslib=">, Group<m_Group>;
 def mpascal_strings : Flag<["-"], "mpascal-strings">, Alias<fpascal_strings>;
 def mred_zone : Flag<["-"], "mred-zone">, Group<m_Group>;
+def mno_tls_load_hoist : Flag<["-"], "mno-tls-load-hoist">, Group<m_Group>,
+  HelpText<"Disable hoisting TLS address">;
+def mtls_load_hoist : Flag<["-"], "mtls-load-hoist">, Group<m_Group>, Flags<[CC1Option]>,
+  HelpText<"Hoist fetching TLS address in PIC model to remove redundant TLS address call">,
+  MarshallingInfoFlag<CodeGenOpts<"TlsAddrLoadHoist">>;
 def mtls_direct_seg_refs : Flag<["-"], "mtls-direct-seg-refs">, Group<m_Group>,
   HelpText<"Enable direct TLS access through segment registers (default)">;
 def mregparm_EQ : Joined<["-"], "mregparm=">, Group<m_Group>;
 def mrelax_all : Flag<["-"], "mrelax-all">, Group<m_Group>, Flags<[CC1Option,CC1AsOption]>,
   HelpText<"(integrated-as) Relax all machine instructions">,
   MarshallingInfoFlag<CodeGenOpts<"RelaxAll">>;
 def mincremental_linker_compatible : Flag<["-"], "mincremental-linker-compatible">, Group<m_Group>,
   Flags<[CC1Option,CC1AsOption]>,

clang/lib/CodeGen/CGCall.cpp

if (!HasOptnone) {
     if (CodeGenOpts.OptimizeSize == 2)
       FuncAttrs.addAttribute(llvm::Attribute::MinSize);
   }
 
   if (CodeGenOpts.DisableRedZone)
     FuncAttrs.addAttribute(llvm::Attribute::NoRedZone);
   if (CodeGenOpts.IndirectTlsSegRefs)
     FuncAttrs.addAttribute("indirect-tls-seg-refs");
+  if (CodeGenOpts.TlsAddrLoadHoist && Context.getLangOpts().PICLevel > 0)
+    FuncAttrs.addAttribute("tls-load-hoist");
   if (CodeGenOpts.NoImplicitFloat)
     FuncAttrs.addAttribute(llvm::Attribute::NoImplicitFloat);
 
   if (AttrOnCallSite) {
     // Attributes that should go on the call site only.
     if (!CodeGenOpts.SimplifyLibCalls || LangOpts.isNoBuiltinFunc(Name))
       FuncAttrs.addAttribute(llvm::Attribute::NoBuiltin);
     if (!CodeGenOpts.TrapFuncName.empty())

clang/lib/Driver/ToolChains/Clang.cpp

if (Args.hasFlag(options::OPT_fno_operator_names,
     CmdArgs.push_back("-fno-operator-names");
 
   // Forward -f (flag) options which we can pass directly.
   Args.AddLastArg(CmdArgs, options::OPT_femit_all_decls);
   Args.AddLastArg(CmdArgs, options::OPT_fheinous_gnu_extensions);
   Args.AddLastArg(CmdArgs, options::OPT_fdigraphs, options::OPT_fno_digraphs);
   Args.AddLastArg(CmdArgs, options::OPT_femulated_tls,
                   options::OPT_fno_emulated_tls);
+
+  if (Args.hasFlag(options::OPT_mtls_load_hoist,
+                   options::OPT_mno_tls_load_hoist, false))
+    CmdArgs.push_back("-mtls-load-hoist");
+
   Args.AddLastArg(CmdArgs, options::OPT_fzero_call_used_regs_EQ);
 
   if (Arg *A = Args.getLastArg(options::OPT_fzero_call_used_regs_EQ)) {
     // FIXME: There's no reason for this to be restricted to X86. The backend
     // code needs to be changed to include the appropriate function calls
     // automatically.
     if (!Triple.isX86())
       D.Diag(diag::err_drv_unsupported_opt_for_target)

clang/test/CodeGen/X86/tls_loads.cpp

+// RUN: %clang -target x86_64-unknown-unknown -fPIC -ftls-model=global-dynamic -mtls-load-hoist -O2 -S -o - %s -emit-llvm | FileCheck %s --check-prefix=HOIST
+// RUN: %clang -target x86_64-unknown-unknown -fPIC -ftls-model=global-dynamic -mno-tls-load-hoist -O2 -S -o - %s -emit-llvm | FileCheck %s --check-prefix=NOHOIST
+
+// Use this test generate IR test for llvm/test/CodeGen/X86/intel-tls-loads-control*.ll
+thread_local int thl_x;
+thread_local int thl_x2;
+
+struct SS {
+  char thl_c;
+  int num;
+};
+
+int gfunc();
+int gfunc2(int);
+
+int f1(int c) {
+  int *px = &thl_x;
+  while (c--)
+    *px += gfunc2(thl_x2);
+  return *px;
+}
+
+int f2(int c) {
+  thread_local struct SS st;
+  while (c--) {
+    thl_x += gfunc();
+    st.thl_c += (char)gfunc();
+    st.num += gfunc();
+  }
+  return thl_x;
+}
+
+int f3(int c) {
+  int *px = &thl_x;
+  gfunc2(*px);
+  gfunc2(*px);
+  return 1;
+}
+
+// HOIST: attributes {{.*}}"tls-load-hoist"
+// HOIST: attributes {{.*}}"tls-load-hoist"
+// HOIST: attributes {{.*}}"tls-load-hoist"
+
+// NOHOIST-NOT: attributes {{.*}}"tls-load-hoist"

llvm/docs/LangRef.rst

 ``"thunk"``
     This attribute indicates that the function will delegate to some other
     function with a tail call. The prototype of a thunk should not be used for
     optimization purposes. The caller is expected to cast the thunk prototype to
     match the thunk target prototype.
 
 ``"tls-load-hoist"``
     This attribute indicates that the function will try to reduce redundant
-    tls address caculation by hoisting tls variable.
+    tls address calculation by hoisting tls variables.
 
 ``uwtable[(sync|async)]``
     This attribute indicates that the ABI being targeted requires that
     an unwind table entry be produced for this function even if we can
     show that no exceptions passes by it. This is normally the case for
     the ELF x86-64 abi, but it can be disabled for some compilation
     units. The optional parameter describes what kind of unwind tables
     to generate: ``sync`` for normal unwind tables, ``async`` for asynchronous

llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp

 #include <tuple>
 #include <utility>
 
 using namespace llvm;
 using namespace tlshoist;
 
 #define DEBUG_TYPE "tlshoist"
 
-// TODO: Support "strict" model if we need to strictly load TLS address,
-// because "non-optimize" may also do some optimization in other passes.
-static cl::opt<std::string> TLSLoadHoist(
-    "tls-load-hoist",
-    cl::desc(
-        "hoist the TLS loads in PIC model: "
-        "tls-load-hoist=optimize: Eleminate redundant TLS load(s)."
-        "tls-load-hoist=strict: Strictly load TLS address before every use."
-        "tls-load-hoist=non-optimize: Generally load TLS before use(s)."),
-    cl::init("non-optimize"), cl::Hidden);
+static cl::opt<bool> TLSLoadHoist(
+    "tls-load-hoist", cl::init(false), cl::Hidden,
+    cl::desc("hoist the TLS loads in PIC model to eleminate redundant "
+             "TLS address calculation."));
 
 namespace {
 
 /// The TLS Variable hoist pass.
 class TLSVariableHoistLegacyPass : public FunctionPass {
 public:
   static char ID; // Pass identification, replacement for typeid
 

 }
 
 /// Optimize expensive TLS variables in the given function.
 bool TLSVariableHoistPass::runImpl(Function &Fn, DominatorTree &DT,
                                    LoopInfo &LI) {
   if (Fn.hasOptNone())
     return false;
 
-  if (TLSLoadHoist != "optimize" &&
-      !Fn.getAttributes().hasFnAttr("tls-load-hoist"))
+  if (!TLSLoadHoist && !Fn.getAttributes().hasFnAttr("tls-load-hoist"))
     return false;
 
   this->LI = &LI;
   this->DT = &DT;
   assert(this->LI && this->DT && "Unexcepted requirement!");
 
   // Collect all TLS variable candidates.
   collectTLSCandidates(Fn);

llvm/test/CodeGen/X86/tls-loads-control.ll

-; RUN: llc -mtriple=x86_64-unknown-unknown -O2 --relocation-model=pic --tls-load-hoist=optimize --stop-after=tlshoist -o - %s | FileCheck %s
+; RUN: llc -mtriple=x86_64-unknown-unknown -O2 --relocation-model=pic --tls-load-hoist=true --stop-after=tlshoist -o - %s | FileCheck %s
 ; RUN: llc -mtriple=x86_64-unknown-unknown -O2 --relocation-model=pic --stop-after=tlshoist -o - %s | FileCheck %s
 
 ; This test come from compiling clang/test/CodeGen/intel/tls_loads.cpp with:
 ; (clang tls_loads.cpp -fPIC -ftls-model=global-dynamic -O2 -S -emit-llvm)
 
 ; // Variable declaration and definition:
 ; thread_local int thl_x;
 ; thread_local int thl_x2;

llvm/test/CodeGen/X86/tls-loads-control2.ll

-; RUN: opt -S -mtriple=x86_64-unknown-unknown -tlshoist --relocation-model=pic --tls-load-hoist=optimize -o - %s | FileCheck %s --check-prefix=HOIST0
-; RUN: opt -S -mtriple=x86_64-unknown-unknown -tlshoist --relocation-model=pic --tls-load-hoist=non-optimize -o - %s | FileCheck %s --check-prefix=HOIST2
+; RUN: opt -S -mtriple=x86_64-unknown-unknown -tlshoist --relocation-model=pic --tls-load-hoist=true -o - %s | FileCheck %s --check-prefix=HOIST0
 ; RUN: opt -S -mtriple=x86_64-unknown-unknown -tlshoist --relocation-model=pic -o - %s | FileCheck %s --check-prefix=HOIST2
 
 $_ZTW5thl_x = comdat any
 
 @thl_x = thread_local global i32 0, align 4
 
 ; Function Attrs: mustprogress uwtable
 define i32 @_Z2f1i(i32 %c) local_unnamed_addr #0 {

llvm/test/CodeGen/X86/tls-loads-control3.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc -mtriple=x86_64-unknown-unknown -O2 --relocation-model=pic --tls-load-hoist=optimize -o - %s | FileCheck %s --check-prefix=HOIST0
-; RUN: llc -mtriple=x86_64-unknown-unknown -O2 --relocation-model=pic --tls-load-hoist=non-optimize -o - %s | FileCheck %s --check-prefix=HOIST2
+; RUN: llc -mtriple=x86_64-unknown-unknown -O2 --relocation-model=pic --tls-load-hoist=true -o - %s | FileCheck %s --check-prefix=HOIST0
 ; RUN: llc -mtriple=x86_64-unknown-unknown -O2 --relocation-model=pic -o - %s | FileCheck %s --check-prefix=HOIST2
 
 ; This test has no module flag {"tls-load-hoist", i32 0}, so use --tls-load-hoist=x
 ; to choose the way of loading thread_local address.
 
 ; This test come from compiling clang/test/CodeGen/intel/tls_loads.cpp with:
 ; (clang tls_loads.cpp -fPIC -ftls-model=global-dynamic -O2 -S -emit-llvm)