[PATCH] D28913: ThinLTOBitcodeWriter: Strip debug info from merged module.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

This LGTM as a temporary approach - but please wait for Mehdi. Was https://reviews.llvm.org/D29240 the fix of the root cause of the bot failure? Is this fix required to work around a breakage now, or just an optimization to remove unnecessary debug info?



================
Comment at: llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:265
   std::unique_ptr<Module> MergedM(CloneModule(&M, VMap, IsInMergedM));
+  StripDebugInfo(*MergedM);
 
----------------
krasin wrote:
> pcc wrote:
> > mehdi_amini wrote:
> > > pcc wrote:
> > > > mehdi_amini wrote:
> > > > > pcc wrote:
> > > > > > mehdi_amini wrote:
> > > > > > > Debug info is costly to manipulate, can't we have a flag on CloneModule that just ignores them in the first place?
> > > > > > Maybe we can do that in a follow up change. For the moment it seems simplest to strip after cloning.
> > > > > Are you expecting this to be involved? 
> > > > > 
> > > > > 
> > > > We would need to thread the flag through to practically everywhere we create IR when cloning a module. Not that involved but certainly more than a one liner. (And the perf improvement associated with adding the flag hasn't been measured.)
> > > > 
> > > > I've started looking at the bug Ivan found. In the meantime, it would be nice if we could land this change to unbreak our bot.
> > > Which bot? Chromium bot? Why don't you patch this internally?
> > > 
> > > Which bot? Chromium bot?
> > 
> > Yes.
> > 
> > > Why don't you patch this internally?
> > 
> > We don't normally do that.
> This is a Clang ToT bot that is aimed to track the pristine LLVM tree. So, it is more like a bot on labs.llvm.org than other Chromium bots.
> 
> One reason to unbreak the bot even if it's a workaround for now would be to make sure we don't get unrelated regressions related to ThinLTO / LLD / Debug info on the LLVM side.
Suggest adding a comment about why this is safe, and a FIXME to have CloneModule optionally ignore the debug info. 


https://reviews.llvm.org/D28913