[PATCH] D49362: [ThinLTO] Internalize read only globals

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D49362#1289042, @evgeny777 wrote:

> > I'm not sure why we couldn't import and make a local copy of the aliasee when an alias to it is preserved and/or not eligible to import
>
> My guess is because alias and aliasee are in the fact the same object (both point to the same memory location). Now assume alias is preserved and visible outside of the DSO. One can modify the object via alias (during program initialization for example) which
>  will take no effect on aliasee if it's been imported to a different module and internalized. The same applies when alias is listed in `llvm.used`


Ok, that makes sense. For normal importing, we don't need to look at the importability of the alias on a reference to the aliasee since we will promote it. The difference here is that these conditions on the alias mean we don't have visibility into possible writes to the aliasee via the alias, and thus can't safely mark it read only. Can you add a comment to this effect in propagateConstants where you are checking those conditions?

I think it would be clearer in propagateConstants to do the checks only on the applicable summary types:

1. liveness check only on global var summaries
2. the importability checks on either global var or alias summaries (with note as suggested above about why on alias summaries)
3. the call to propagateConstantsToRefs only on function summaries

It works without the checks, but I think that guarding with the appropriate checks + a comment as to why would aid in understanding.


https://reviews.llvm.org/D49362