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

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.



> 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?

Yes, this is the crash with the test-case in this patch.

>> - 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.

I have a different approach: 1) this should go in in any case, because correctness first, and 2) it does not seem that much "long term", it is even fairly easy to implement in the importer, but I would prioritize it depending on the benchmarks.


https://reviews.llvm.org/D26880