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