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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added a comment.

Did anyone sign off on the new insertion position code? It looks like the only approval is from before you added that. This should have been moved back to needs review.



================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:190
+
+Instruction *TLSVariableHoistPass::getNearestLoopDomInst(BasicBlock *BB) {
+  Loop *L = LI->getLoopFor(BB);
----------------
Why not pass the Loop* you already looked up in the caller. Why get the loop again?


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:194
+
+  // Get the outmost loop.
+  while (Loop *Parent = L->getParentLoop())
----------------
outmost -> outermost


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:201
+  // There is unique predecessor outside the loop.
+  // Note the terminator maybe nullptr, because the PreHeader maybe an empty BB.
+  if (PreHeader)
----------------
I'm not sure that's true. Control flow in IR is explicit, there is no fallthrough. If it's a predecessor it must have a branch instruction of some kind.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120000