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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
