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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: llvm/include/llvm/IR/Module.h:914
+  /// Get/set the model of TLS address loading.
+  StringRef getTlsAddrLoadHoist() const;
+  void setTlsAddrLoadHoist(StringRef Model);
----------------
craig.topper wrote:
> Why a module flag? What is the policy for LTO merging?
Because the TLS is Global Value which has Scope for module.
I am not clear about the LTO merging, I think one module's flag should not "spread" to another module.


================
Comment at: llvm/include/llvm/Transforms/Scalar/TLSVariableHoist.h:122
+
+  /// Use GVs to make sure the order of TLS Global Varibles.
+  SmallVector<GlobalVariable *> GVs;
----------------
craig.topper wrote:
> Varibles -> Variables.
> 
> Can we use llvm::MapVector to avoid a separate  SmallVector?
Let me check the MapVector, rarely use it before : )  thanks a lot!


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:131
+    if (TLSCandMap.count(GV) == 0) {
+      tlshoist::TLSCandidate Candidate;
+      Candidate.addUser(Inst, Idx);
----------------
craig.topper wrote:
> The GV field in TLSCandidate isn't assigned is it?
Yes, I should remove the "GV" field from  TLSCandidate. 


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:136
+    } else {
+      TLSCandMap[GV].addUser(Inst, Idx);
+    }
----------------
craig.topper wrote:
> `TLSCandMap[GV].addUser(Inst, Idx);` works even if GV isn't in the map. The entry will be default constructed before addUser is called.
Yes, So here the "else" means TLSCandMap.count(GV) != 0 (GV is in the map)


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:145
+  bool HasTLS = false;
+  for (GlobalVariable &GV : M->globals()) {
+    if (GV.isThreadLocal()) {
----------------
craig.topper wrote:
> can we use llvm::any_of here?
Yes, I checked it, that is good, I never use it before : )


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:186
+  BasicBlock &Entry = Fn.getEntryBlock();
+  BasicBlock::iterator Iter = Entry.getFirstInsertionPt();
+  Type *Ty = GV->getType();
----------------
craig.topper wrote:
> Is this before the allocas?
Sorry, don't much understand,
What the problem if it before allocas ? 
This is in IR level and the Global Value do not need "allocas" 


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:218
+  for (auto *GV : GVs)
+    Replaced = tryReplaceTLSCandidate(Fn, GV) || Replaced;
+
----------------
craig.topper wrote:
> `Replaced |= tryReplaceTLSCandidate(Fn, GV);`
I think "|=" is a bit operation. Here is bool, the stardard way is "||"


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:226
+                                   LoopInfo &LI) {
+  bool MadeChange = false;
+  Module *M = Fn.getParent();
----------------
craig.topper wrote:
> Should we call skipFunction() for opt-bisect-limit and optnone support?
1 Yes, we should consider skipFunction, good catch!
2 **optnone ** will not created this pass at TargetPassConfig.cpp


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:228
+  Module *M = Fn.getParent();
+  if (TLSLoadHoist != "Optimize" && M->getTlsAddrLoadHoist() != "Optimize")
+    return MadeChange;
----------------
craig.topper wrote:
> Do we normally use capitalized strings?
I checked the other similar uses, we normally not use capitalized strings, let me change it, thanks a lot!


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

https://reviews.llvm.org/D120000