[PATCH] D42856: [ThinLTO] Convert dead alias to declarations
Teresa Johnson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Feb 5 06:39:08 PST 2018
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
More information about the llvm-commits
mailing list