[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/IR/Module.h:914
+  /// Get/set the model of TLS address loading.
+  StringRef getTlsAddrLoadHoist() const;
+  void setTlsAddrLoadHoist(StringRef Model);
----------------
xiangzhangllvm wrote:
> 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.
The first thing LTO does is merges all modules into a single module. Then the optimization pipeline runs on that unified module.

If we made it a function attribute would anything break? Could two different functions have a different hoisting policy? That would avoid the LTO issue.


================
Comment at: llvm/lib/IR/Module.cpp:738
+  MDString *ID = MDString::get(getContext(), Model);
+  addModuleFlag(ModFlagBehavior::Error, "tls-load-hoist", ID);
+}
----------------
I believe the `ModFlagBehavior::Error` here controls what happens in LTO. It will fail to merge if the modules have different flags.


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:186
+  BasicBlock &Entry = Fn.getEntryBlock();
+  BasicBlock::iterator Iter = Entry.getFirstInsertionPt();
+  Type *Ty = GV->getType();
----------------
xiangzhangllvm wrote:
> 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" 
The alloca instructions for the function's local variables are the first instructons in the entry basic block. Not if we should be putting the bitcast before them.


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


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:226
+                                   LoopInfo &LI) {
+  bool MadeChange = false;
+  Module *M = Fn.getParent();
----------------
xiangzhangllvm wrote:
> 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
There is an optnone function attribute which is different than the global optlevel.


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

https://reviews.llvm.org/D120000