[PATCH] D13515: Support for ThinLTO function importing and symbol linking.

Teresa Johnson via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 16 14:01:35 PDT 2015


tejohnson added inline comments.

================
Comment at: include/llvm/IR/FunctionInfo.h:235
@@ +234,3 @@
+                                         getModuleId(M->getModuleIdentifier()));
+      for (const auto &FI : getFunctionInfoList(FuncName)) {
+        if (M->getModuleIdentifier() == FI->functionSummary()->modulePath())
----------------
davidxl wrote:
> tejohnson wrote:
> > davidxl wrote:
> > > comdat functions should have been merged at this point, right?
> > Merged where? At the point we invoke this we are just getting a module ready to import from - we would not have merged comdats across modules. And the FunctionInfoList may contain multiple copies of a comdat, unless we decided in the stage-2 global indexing step to merge their entries in the combined index. Presumably due to the way this is being used to guard renaming and promotion of locals, we couldn't remove an entry for any comdat copy that accessed a local - but I doubt that could happen?
> yes, In theory, the combined index generator can do comdat de-duplication before writing out the combined summary -- but since that step is mandatory, handling duplication is still required here.
Right, that is what this loop is handling.

================
Comment at: lib/Linker/LinkModules.cpp:676
@@ +675,3 @@
+  // can simply use the clone created in this module.
+  if (GVar && GVar->isConstant()) return false;
+  return true;
----------------
tejohnson wrote:
> davidxl wrote:
> > tobiasvk wrote:
> > > tejohnson wrote:
> > > > davidxl wrote:
> > > > > Unless the address taken bit is set (to handle the case address is used in comparison).
> > > > Good catch. And unfortunately for GVars there is no address taken bit or easy check for this. For functions, there is a hasAddressTaken function that walks the uses, but not for GVars. But that isn't useful here in the importing context in any case, since we haven't even parsed all of the functions where it may be address taken. It will have to be recorded in the function index in the front end. For now I will simply promote all locals (and leave a comment about what would need to be done to reduce this scope).
> > > Could you use the unnamed_addr bit?
> > When promoting Constant Global, make sure the initializer is made available so that const prop can still happen.
> Thanks for the tip!
> 
> I am not familiar with this attribute, but see it documented at http://llvm.org/docs/LangRef.html#global-variables:
> 
> "Global variables can be marked with unnamed_addr which indicates that the address is not significant, only the content. Constants marked like this can be merged with other constants if they have the same initializer. Note that a constant with significant address can be merged with a unnamed_addr constant, the result being a constant whose address is significant."
> 
> So it sounds like unnamed_addr variables are safe to be treated as non-address taken. I'm not sure under what context it is set though or if it covers all non-address taken const variables?
> When promoting Constant Global, make sure the initializer is made available so that const prop can still happen.
Right, the initializers are imported along with the variables.


http://reviews.llvm.org/D13515





More information about the llvm-commits mailing list