[PATCH] D120000: [1/2] TLS loads opimization (hoist)

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Scalar/TLSVariableControl.h:118
+
+  void collectTLSCandidates(Function &Fn);
+  void collectTLSCandidate(Instruction *Inst);
----------------
craig.topper wrote:
> Should these methods be private?
Make sense, look no need to directly call it though pass obj.


================
Comment at: llvm/include/llvm/Transforms/Scalar/TLSVariableControl.h:124
+private:
+  DominatorTree *DT;
+  LoopInfo *LI;
----------------
I prefer to keep the DT here for refine the insert point later.


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableControl.cpp:1
+//===- TLSVariableControl.cpp - Remove Redundant TLS Loads --------===//
+//
----------------
craig.topper wrote:
> TLSVariableControl doesn't seem like a good name for the pass if the description doesn't use the words "variable" or "control". Does "control" have some meaning with TLS?
"control" has no meaning with TLS, let refine it to "hoist" () ?
but "variable" I think it is OK here.
May be better to rename as "TLSVariableHoist " ? or you have better names? thanks!


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableControl.cpp:94
+             "tls-load-control=Optimize: Eleminate remove redundant TLS load(s)."
+             "tls-load-control=Strict: Strictly load TLS address before every use."
+             "tls-load-control=Non-Optimize: Generally load TLS before use(s)."),
----------------
pengfei wrote:
> `Strict` is not handled?
Yes, current not very much sure if we will have the requirement for strictly call tls-address function every time. 
So I just preserve it here.


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableControl.cpp:115
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.setPreservesCFG();
+    AU.addRequired<DominatorTreeWrapperPass>();
----------------
pengfei wrote:
> setPreservesAll?
the Pass is very near ISel, I think no need to preserves all.


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableControl.cpp:148-149
+  bool MadeChange =
+      Impl.runImpl(Fn, getAnalysis<DominatorTreeWrapperPass>().getDomTree(),
+                   getAnalysis<LoopInfoWrapperPass>().getLoopInfo());
+
----------------
pengfei wrote:
> Move `getAnalysis` later in the function.
TLSVariableControlPass do not inherit FunctionPass,  So can not directly use it.


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableControl.cpp:176
+    if (TLSCandMap.count(GV) == 0) {
+      tlscontrol::TLSCandidate Candidate(GV);
+      Candidate.addUser(Inst, Idx);
----------------
pengfei wrote:
> No need namespace.
Because TLSCandidate  is defined outside of TLSVariableControlPass , So I add tlscontrol to make it more safe.


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableControl.cpp:178
+      Candidate.addUser(Inst, Idx);
+      TLSCandMap[GV] = Candidate;
+      GVs.push_back(GV);
----------------
pengfei wrote:
> GV has been used as Key, no need store in Candidate.
Yes, make sense, and we can get the GV by passing GV in the function arguments. 

There is some story here about GVs too, first I didn't add GVs , but later I find the order of iter GV in
"for (GV : TLSCandMap.keys)"  is not immutable， that will take a lot trouble for checking the lit test. SO I keep the GV in GVs.


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableControl.cpp:294-296
+  PreservedAnalyses PA;
+  PA.preserveSet<CFGAnalyses>();
+  return PA;
----------------
pengfei wrote:
> Do we break any analyse info? Can we return `PreservedAnalyses::all()`?
This pass is very later at Scalar optmization. I think we no need to preserve all. Let me carefully re-consider here, thanks!


================
Comment at: llvm/test/CodeGen/X86/tls-loads-control.ll:1
+; RUN: opt -S -mtriple=x86_64-unknown-unknown -tlscontrol --relocation-model=pic --tls-load-control=Optimize -o - %s | FileCheck %s --check-prefixes=CONTROL0
+; RUN: opt -S -mtriple=x86_64-unknown-unknown -tlscontrol --relocation-model=pic -o - %s | FileCheck %s --check-prefixes=CONTROL0
----------------
pengfei wrote:
> Don't need since both check the same.
The test contain "!3 = !{i32 1, !"tls-load-control", !"Optimize"}" , it has same functonality with "--tls-load-control=Optimize"
But test both of them here is not bad, I think.


================
Comment at: llvm/test/CodeGen/X86/tls-loads-control.ll:11-13
+$_ZTW5thl_x = comdat any
+
+$_ZTW6thl_x2 = comdat any
----------------
pengfei wrote:
> What are they used for?
This test is directly generate from a clang test I'll commit latter.
So let it be the raw result of source file out put (I comment at line 8) is good to check the source of it.
So I didn't manually change it more.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120000/new/

https://reviews.llvm.org/D120000