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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


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


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:634
+    else
+      NewGV =
+          new GlobalVariable(*GV.getParent(), GV.getValueType(),
----------------
Looks `else` part is not covered by any tests atm.
(I replaced it with `else assert(false)` and check-all showed no failtures).

Suggest to add variable with alias to your testcase 
and check them are converted to declarations, like:

```
; RUN:   -r %t1.bc,var,x -r %t1.bc,varAlias,x \
; RUN:   -o %t2.o -save-temps
...
; DROP-DAG: @var = external global i32
; DROP-DAG: @varAlias = external global i32
...
@var = global i32 99
@varAlias = alias i32, i32* @var
```


Repository:
  rL LLVM

https://reviews.llvm.org/D42856