[PATCH] D42107: [ThinLTO] - Stop internalizing and drop non-prevailing symbols.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: include/llvm/Transforms/IPO/FunctionImport.h:117
 /// in the graph from any of the given symbols listed in
-/// \p GUIDPreservedSymbols.
+/// \p GUIDPreservedSymbols. Non-prevailing symbols that are not anywhere in IR
+/// are normally "dead", \p isPrevailing predicate returns status of symbol.
----------------
tejohnson wrote:
> Suggest changing "that are not anywhere in IR are normally dead" to something like "are symbols without a prevailing copy anywhere in IR and are normally dead". I.e. a non-prevailing symbol here can have a def in IR, it is just not prevailing.
Done.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:501
+    const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols,
+    function_ref<PrevailingType(GlobalValue::GUID)> isPrevailing) {
   assert(!Index.withGlobalValueDeadStripping());
----------------
tejohnson wrote:
> I think you lost the corresponding change to the header declaration in this version of the patch.
I think it is still there, right below text change you suggested :)

```
/// Compute all the symbols that are "dead": i.e these that can't be reached
/// in the graph from any of the given symbols listed in
/// \p GUIDPreservedSymbols. Non-prevailing symbols are symbols without a
/// prevailing copy anywhere in IR and are normally dead, \p isPrevailing
/// predicate returns status of symbol.
```

I removed comment done on the declaration of `Worklist` though, because after introducing enum
that part of code was reverted to original and there is no more pair with bool.


https://reviews.llvm.org/D42107