[PATCH] D31735: ThinLTOBitcodeWriter: delete comdats if their keys are renamed

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

> Maybe thinlto is internalizing globals without removing their comdats?

This pass is being run in clang prior to internalization, which is done in the linker.

> Also if I read correctly your test-case, the symbol @foo isn't part of the comdat $foo?

Aliases are part of the same comdat as their aliasee. The test case seems valid to me.

> I'd like to push to get the rename implemented. If we have an internal global with a comdat group, the frontend probably put it there for a good reason (the non-leader is probably metadata) and we shouldn't mess with it.

If we remove comdat from an internal global, it will become a GC root. So we will handle it and its metadata correctly, but less optimally.

---

Actually now that I think about it, we would still have the asan globals problem for vtables with external linkage. So I suspect that we may have to move all members of externally visible comdats to the merged module if any member needs to be moved. That is orthogonal to this change because we would still need to handle internal comdats, for example by dropping them, as this change is doing.



================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:79
+        if (ComdatsToDelete.count(C->getName()))
+          GO->setComdat(nullptr);
+        else if (C->getName() != ExportGV.getName())
----------------
inglorion wrote:
> pcc wrote:
> > I think it would be simpler to do two passes over global_values, one where you decide which comdats to delete and the other where you delete them.
> Definitely simpler. Are you ok with iterating all the globals again? This more complex implementation tries to avoid some of the work by immediately deleting references to comdats already slated for deletion and indexing globals by comdat to avoid having to iterate over all the globals again (based on the assumption that most globals either won't have a comdat or the comdat will have the same name as the global).
> 
> Also, did you want me to iterate over the symbols in ImportM as well (as per your "remove the comdats from both modules")?
> Definitely simpler. Are you ok with iterating all the globals again?

Fine with me, modulo this being the right place for the fix.

> Also, did you want me to iterate over the symbols in ImportM as well (as per your "remove the comdats from both modules")?

I think the best place to do it is in a separate function, where this function is responsible for collecting and the other one is responsible for removing. Basically the caller would do something like this:
```
  std::set<Comdat *> ComdatsToRemove;
  promoteInternals(*MergedM, M, ModuleId, ComdatsToRemove);
  promoteInternals(M, *MergedM, ModuleId, ComdatsToRemove);

  removeInternalComdats(M, ComdatsToRemove);
  removeInternalComdats(*MergedM, ComdatsToRemove);
```


https://reviews.llvm.org/D31735