[PATCH] D42856: [ThinLTO] Convert dead alias to declarations

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 2 inline comments as done.
tejohnson added inline comments.


================
Comment at: lib/LTO/LTOBackend.cpp:414
+    // Increment iterator here since MaybeDrop can delete aliases.
+    MaybeDrop(*I++);
 }
----------------
grimar wrote:
> With the new change `convertToDeclaration` becomes a dangerous function to use in loops.
> 
> Currently there is one more place where code iterates over aliases in a "old" way and may also fail I think:
> https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L670
> https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/IPO/FunctionImport.cpp#L689
> 
> What about if we stop calling `eraseFromParent` from `convertToDeclatation`
> and do something explicit like:
> 
> ```
> static void dropDeadSymbols(Module &Mod, const GVSummaryMapTy &DefinedGlobals,
>                             const ModuleSummaryIndex &Index) {
>   std::vector<GlobalValue *> ReplacedGlobals;
>   for (auto &GV : Mod.global_values())
>     if (GlobalValueSummary *GVS = DefinedGlobals.lookup(GV.getGUID()))
>       if (!Index.isGlobalValueLive(GVS))
>         if (!convertToDeclaration(GV))
>           ReplacedGlobals.push_back(&GV);
> 
>   for (GlobalValue *GV : ReplacedGlobals)
>     GV->eraseFromParent();
> }
> ```
> 
> So `convertToDeclaration` would return true if value was converted to declaration and
> false if it was replaced.
> 
> Though we can refactor this place separatelly in a followups, probably with use of 
> some better approach, but at least caller from FunctionImport.cpp should also be changed too then.
I changed as suggested as that is less problematic for other callsites.
Note that there won't currently be a problem at the other callsite since it doesn't call convertToDeclaration with an alias (otherwise it previously would have hit the lllvm_unreachable). I added an assert there that convertToDeclaration returns true, and a fixme that it should be changed once thinLTOResolveWeakForLinkerGUID is changed to handle aliases (which it can now be with this change in convertToDeclaration).


Repository:
  rL LLVM

https://reviews.llvm.org/D42856