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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D49362#1292864, @tejohnson wrote:

> In https://reviews.llvm.org/D49362#1292863, @evgeny777 wrote:
>
> > > Why won't it work for distributed builds? The read only gvar flag is being serialized out and back in, so shouldn't it show up in the distributed indexes?
> >
> > It does, but we can't be sure about "read-only" unless we analyze function references. And we need propagateConstants for that.
>
>
> propagateConstants should run during the thin link, which would happen in a distributed build before writing indexes. Maybe there is something weird about that test where it's not actually running the real thin link, will look...


Hmm that test is using llvm-lto2 to run the thin link and write distributed indexes, which should do the full thin link. Can you look at why it isn't running propagateConstants during the thin link?

That being said, your change in https://reviews.llvm.org/D54306 seems fine in that we wouldn't have run propagateConstants if the dead stripping bit in the index isn't set. But there again, we should have run dead stripping and therefore propagateConstants during the thin link of llvm-lto2, set the with dead stripping bit in the index, and serialized that through the distributed indexes. Can you see why that isn't being run and set in the distributed indexes (both the with dead stripping flag and the read only flags)?


https://reviews.llvm.org/D49362