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

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm 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);
----------------
craig.topper wrote:
> 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.
OK, let me move it into function attribute. Thanks a lot!


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:186
+  BasicBlock &Entry = Fn.getEntryBlock();
+  BasicBlock::iterator Iter = Entry.getFirstInsertionPt();
+  Type *Ty = GV->getType();
----------------
craig.topper wrote:
> 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.
OK, Let me re-place the bitcast position.


================
Comment at: llvm/lib/Transforms/Scalar/TLSVariableHoist.cpp:226
+                                   LoopInfo &LI) {
+  bool MadeChange = false;
+  Module *M = Fn.getParent();
----------------
craig.topper wrote:
> 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.
OK, let me check it again, but for skipFunction, I can't call it here, because the TLSVariableHoistPass not inherit FunctionPass,
but I already check it at TLSVariableHoistLegacyPass::runOnFunction.


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

https://reviews.llvm.org/D120000