This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Refine fix to avoid renaming of uses in inline assembly.
ClosedPublic

Authored by tejohnson on Apr 26 2016, 5:47 PM.

Details

Summary

Refine the workaround from r266877 that attempts to prevent
renaming of locals in inline assembly, so that in addition to looking
for a llvm.used local value, that there is at least one inline assembly
call in the module. Otherwise, debug functions added to the llvm.used
can block importing/exporting unnecessarily.

Diff Detail

Event Timeline

tejohnson updated this revision to Diff 55143.Apr 26 2016, 5:47 PM
tejohnson retitled this revision from to [ThinLTO] Refine fix to avoid renaming of uses in inline assembly..
tejohnson updated this object.
tejohnson added a reviewer: mehdi_amini.
tejohnson added a subscriber: llvm-commits.
mehdi_amini accepted this revision.Apr 26 2016, 5:55 PM
mehdi_amini edited edge metadata.

LGTM.
This is sketchy though, I'm fine with it on the short term, but anything "clever" using the summary will assume that if you have a summary you have all the informations in the summary for the module somehow, which this is breaking AFAICT.

lib/Analysis/ModuleSummaryAnalysis.cpp
151

I think there is a shortcut for (auto &I : F.instructions())

This revision is now accepted and ready to land.Apr 26 2016, 5:55 PM
In D19573#413169, @joker.eph wrote:

LGTM.
This is sketchy though, I'm fine with it on the short term, but anything "clever" using the summary will assume that if you have a summary you have all the informations in the summary for the module somehow, which this is breaking AFAICT.

I don't think it is quite that bad, since in this case we completely prevent building a summary for the module in question, so it should not have a different effect than linking it in as a native object file (where the rest of the bitcode files would have no ability to import from it etc either).

In any case, I do plan to move this to the more refined solution soon, but it isn't at the top of my heap just yet.

lib/Analysis/ModuleSummaryAnalysis.cpp
151

Thanks for the pointer (actually instructions(F), from InstIterator.h)

tejohnson updated this revision to Diff 55218.Apr 27 2016, 7:14 AM
tejohnson edited edge metadata.
  • Implement review suggestion (and rebase).
This revision was automatically updated to reflect the committed changes.