[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#600745, @mehdi_amini wrote:

> So my current feeling is:
>
> - the fix for PR31052 is to disable the "magic" in IRLinker that import definition we didn't ask for.


The IRLinker shouldn't be changed - it would be removing the constant unnamed_addr handling from doPromoteLocalToGlobal. The IRLinker does the right thing based on the linkage.

> - let this patch go

I think you would also need this patch to go in so globalvar refs wouldn't then be marked exported, which presumably is causing an issue when the exporting (defining) module tried to promote those with sections?

> - work on the importing logic to start considering "refs" to be imported when possible / profitable (i.e. catch the performance gain we hope to have with PR31052)

This is the right long term fix. However I am concerned about performance we may give up in the meantime (PR31052 is  fixing a case where we already are getting optimized code). I'd prefer to keep the current approach in the meantime, unless benchmarking shows that it doesn't have significant impact. It would mean changing this fix to instead simply do a recursive walk of global var refs in eligibleForImport, along with my current fix for PR31052.


https://reviews.llvm.org/D26880