[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#601859, @tejohnson wrote:

> In https://reviews.llvm.org/D26880#601841, @mehdi_amini wrote:
>
> > 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.


I collected SPEC performance data with the current optimization on (default) and disabled (in doPromoteLocalToGlobal), and there was no consistent performance delta. So I'll drop my objection to removing this optimization for now until we can do it in the thin link via the summary.

However, please update this patch to remove that handling from doPromoteLocalToGlobal and add a test case that would fail with the current patch until the doPromoteLocalToGlobal handling is removed (I suggest start with the test case added in https://reviews.llvm.org/D19102 but make referencedbyglobal a local so that the referencing global var would be imported as a copy instead of promoted - then referencedbyglobal should be referenced but not promoted).


https://reviews.llvm.org/D26880