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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Scalar/TLSVariableHoist.h:64
+
+#include "llvm/ADT/SetVector.h"
+#include "llvm/ADT/SmallVector.h"
----------------
Is SetVector used by this file?


================
Comment at: llvm/include/llvm/Transforms/Scalar/TLSVariableHoist.h:68
+#include "llvm/IR/PassManager.h"
+#include <algorithm>
+#include <map>
----------------
are any algorithms used in this file?


================
Comment at: llvm/include/llvm/Transforms/Scalar/TLSVariableHoist.h:70
+#include <map>
+#include <vector>
+
----------------
Is vector used by this file?


================
Comment at: llvm/include/llvm/Transforms/Scalar/TLSVariableHoist.h:122
+
+  /// Use GVs to make sure the order of TLS Global Varibles.
+  SmallVector<GlobalVariable *> GVs;
----------------
Varibles -> Variables.

Can we use llvm::MapVector to avoid a separate  SmallVector?


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:10
+// This pass identifies/eliminate Redundant TLS Loads if related option is set.
+// The exmaple: PLS refer to the comment at the head of TLSVariableHoist.h.
+//
----------------
exmaple -> example


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:10
+// This pass identifies/eliminate Redundant TLS Loads if related option is set.
+// The exmaple: PLS refer to the comment at the head of TLSVariableHoist.h.
+//
----------------
craig.topper wrote:
> exmaple -> example
PLS -> Please?


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:131
+    if (TLSCandMap.count(GV) == 0) {
+      tlshoist::TLSCandidate Candidate;
+      Candidate.addUser(Inst, Idx);
----------------
The GV field in TLSCandidate isn't assigned is it?


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


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:145
+  bool HasTLS = false;
+  for (GlobalVariable &GV : M->globals()) {
+    if (GV.isThreadLocal()) {
----------------
can we use llvm::any_of here?


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:186
+  BasicBlock &Entry = Fn.getEntryBlock();
+  BasicBlock::iterator Iter = Entry.getFirstInsertionPt();
+  Type *Ty = GV->getType();
----------------
Is this before the allocas?


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:218
+  for (auto *GV : GVs)
+    Replaced = tryReplaceTLSCandidate(Fn, GV) || Replaced;
+
----------------
`Replaced |= tryReplaceTLSCandidate(Fn, GV);`


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:226
+                                   LoopInfo &LI) {
+  bool MadeChange = false;
+  Module *M = Fn.getParent();
----------------
Should we call skipFunction() for opt-bisect-limit and optnone support?


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:228
+  Module *M = Fn.getParent();
+  if (TLSLoadHoist != "Optimize" && M->getTlsAddrLoadHoist() != "Optimize")
+    return MadeChange;
----------------
Do we normally use capitalized strings?


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

https://reviews.llvm.org/D120000