[clang] [lld] [llvm] [ThinLTO] Reduce the number of renaming due to promotions (PR #178587)

[Teresa Johnson via cfe-commits]

teresajohnson wrote:

> > Ah, this raises an issue I hadn't thought about for distributed ThinLTO. The legacy behavior for distributed ThinLTO is that we import definitions for any and all values with summaries in the sharded index files. So for any values that don't have a corresponding summary, we just import the declaration. Meaning if we decide not to import a local, but import something that references it, we won't get the summary and won't know whether to rename it or not (the default will be to rename). Your force-import-all works around this but is not going to fix the general case. This is going to be an issue for distributed ThinLTO.
> > However, there was not long ago support added for encoding in the summary whether it should be imported as a definition or declaration. This allows us to read the summary for purposes like attribute propagation, and would work well for this case too. But this is currently off by default. I see roughly what we would want to do here to enable importing as a declaration when renaming is off, but I am thinking this is best left as a follow on change. We can disable this now for distributed ThinLTO with a FIXME. But I'm thinking we might want to disable this late, so at least we are testing the path that creates this information. Let me do some testing for a large target to see how much overhead the new analysis adds and get back to you, hopefully in the next ~day. Then we can decide where to disable it.
> 
> Thanks for detailed explanation. I agree that we can delay to support distributed mode for now. Once all related pieces are in place, we can easily support distributed mode.

I did some experiments for ~30 of our large targets. I set -always-rename-promoted-locals to true so that I didn't get linking errors (since we use distributed ThinLTO). I added a separate option to LTO.cpp that when true prevented initializing ExternallyVisibleSymbolNamesPtr when enabled (so that we don't go through any of the analysis in the thin link). I ran the thin link 3x with that new option true and then false and compared the results.

I am seeing a fairly persistent increase in time by 2.5% on average (range is neutral to ~6%) with most increasing 1-3%. Given that the thin link is serial, and for our large targets ever bit helps, I think this should be off by default (enable always rename) at least initially, as it isn't enabling a performance improvement but rather more to simplify the kernel patching. 

To do this, I'd suggest making the AlwaysRenamePromotedLocals option a global in the llvm namespace instead of file static, and using that to prevent the analysis in LTO.cpp, like the following:

```
  DenseSet<StringRef> *ExternallyVisibleSymbolNamesPtr =
      (WholeProgramVisibilityEnabledInLTO && !AlwaysRenamePromotedLocals)
          ? &ExternallyVisibleSymbolNames
          : nullptr;
```

This can then also suffice to keep it off for distributed ThinLTO for now (add a FIXME where the option is defined in FunctionImportUtils.cpp that it needs to be off by default for that reason).

I have a few other minor comments for the patch, let me go through it now and send those, so that with the above change you can wrap this up and commit it.

https://github.com/llvm/llvm-project/pull/178587