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

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.
This revision is now accepted and ready to land.

LGTM, thanks !
(minor suggestions below)



================
Comment at: lib/LTO/LTOBackend.cpp:414
+    // Increment iterator here since MaybeDrop can delete aliases.
+    MaybeDrop(*I++);
 }
----------------
tejohnson wrote:
> 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).
> 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 llvm_unreachable).

Probably yes, though it is also seems can be possible that we do not have such test that would go that path to hit unreachable at this moment.


================
Comment at: lib/LTO/LTOBackend.cpp:410
 
-  // Process functions and global now.
-  // FIXME: add support for aliases (needs support in convertToDeclaration).
-  for (auto &GV : Mod)
-    MaybeDrop(GV);
-  for (auto &GV : Mod.globals())
-    MaybeDrop(GV);
+  for (GlobalValue *GV : ReplacedGlobals) GV->eraseFromParent();
 }
----------------
Please format (it should be 2 lines after clang-formatting I think).


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:684
+      // changed to enable this for aliases.
+      assert(Converted && "Expected GV to be converted");
+    } else {
----------------
I suggest to use llvm_unreachable as we explicitly do not support/implement this path
while assert will be just eliminated in release builds.

```
if (!convertToDeclaration(GV))
  llvm_unreachable("Expected GV to be converted");
```


Repository:
  rL LLVM

https://reviews.llvm.org/D42856