[PATCH] D26880: [ThinLTO] Fix crash: transitively referenced global shouldn't be promoted

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D26880#601841, @mehdi_amini wrote:

> Correct: this optimization is incorrect as is, and not tested, which seems a good reason to remove it :)


The optimization is only incorrect in the transitively referenced case because the handling here and in exportGlobalInModule needs to be recursive. It's easy enough to fix (as shown in https://reviews.llvm.org/D26866 and my proposed patch to fix this case). My other patch https://reviews.llvm.org/D26866 adds a test case and it should be straightforward to modify the test added with the earlier patch that added the code here (https://reviews.llvm.org/D19102) to have regression testing.

> How to recover the optimization is open to discussion though: it seems that we agree on the right thing to do, but you also seem to target a temporary patch along the same line of what I'm removing here. I'm not opposed to the latter, but writing a correct and tested patch to have this optimization seems independent from fixing the correctness issue in the first place.

I think the fix to make this work in the transitive reference case is pretty simple, as is adding tests for it. I am concerned about the cases where we lose performance where this is currently kicking in in the non-transitive case if we remove for some time before a summary-based approach is added.


https://reviews.llvm.org/D26880