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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added a comment.

In D120000#3341406 <https://reviews.llvm.org/D120000#3341406>, @lebedev.ri wrote:

> What are the legality considerations for this transformation,
> when is it legal to perform it? I'm mainly confused why it's opt-in.

This only try hoist TLS address call, the TLS variable will only define in thread function, so it is always legal to do this transformation in a function.
Sorry, what you mean "opt-in" ?



================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:45-52
+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)."),
----------------
lebedev.ri wrote:
> This should be an enum, not strings
yes, we usually use enum here, but I find string is better/easier, because we don't need to define the enum (sometimes may define 2 or more times if we want to use it in clang, for example transfer a clang option to "-mllvm -tls-load-hoist=xxx")


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:135
+    } else {
+      TLSCandMap[GV].addUser(Inst, Idx);
+    }
----------------
craig.topper wrote:
> This line should work for the case when GV isn't already in the map. The operator[] on the MapVector will default construct a TLS candidate before calling addUser. So you don't need to check TLSCandMap.count
Make sense!


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:223
+                                   LoopInfo &LI) {
+  if (Fn.hasOptNone())
+    return false;
----------------
craig.topper wrote:
> This might be handled by the pass manager for the new pass manager. For the old pass manager it is part of skipFunction.
Yes, Both pass manager will call runImpl, it called by "TLSVariableHoistPass::run" and ."TLSVariableHoistLegacyPass::runOnFunction" , (I forget which is new and which is old)


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

https://reviews.llvm.org/D120000