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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:52
+        "tls-load-hoist=non-optimize: Generally load TLS before use(s)."),
+    cl::init("non-optimize"), cl::Hidden);
+
----------------
efriedma wrote:
> Why is this off by default?  Do you plan to turn it on by default in a followup?
Yes, As I answered this question before, the last patch [3/3] will turn it on and update 
affected tests.


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:200
+  // There is unique predecessor outside the loop.
+  // Note the terminator maybe nullptr, because the PreHeader maybe an empty BB.
+  if (PreHeader)
----------------
craig.topper wrote:
> If the Preheader exists, it isn't empty. It will always have a branch to the loop. There are no fallthroughs in IR. So the terminator will not be nullptr.
Eh ..., Yes, Seems make sense, just a question, in which case there will be an empty BB in IR level ? (even the last BB I see is always append with ret instruction) .
 I check the BasicBlock::getTerminator() , it is possible return nullptr.

```
const Instruction *BasicBlock::getTerminator() const {
  if (InstList.empty() || !InstList.back().isTerminator())
    return nullptr;
  return &InstList.back();
}
```


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:239
+  tlshoist::TLSCandidate &Cand = TLSCandMap[GV];
+  if (!DT)
+    return findInsertPosInEntry(Fn, Cand);
----------------
craig.topper wrote:
> Why would DT be null? The pass has DominatorTree as a requirement.
I am not sure if its requirement will must successful build/generate a DominatorTree.
Or here let me change to assert (DT) ?


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:248
+    Instruction *Pos = User.Inst;
+    if (LI)
+      if (Loop *L = LI->getLoopFor(BB)) {
----------------
craig.topper wrote:
> Why would LI be null? The pass has it as a requirement
The same with DT, thanks for your review!


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

https://reviews.llvm.org/D120000