ThinLTOBitcodeWriter: Strip debug info from merged module.

[pcc]

Summary

This module will contain nothing but vtable definitions and (soon)
available_externally function definitions, so there is no point in keeping
debug info in the module.

[pcc]

Ping.

[mehdi_amini]

(Sorry didn't hit send last week)

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

265 ↗

(On Diff #85015)

Debug info is costly to manipulate, can't we have a flag on CloneModule that just ignores them in the first place?

[pcc]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

265 ↗

(On Diff #85015)

Maybe we can do that in a follow up change. For the moment it seems simplest to strip after cloning.

[krasin]

I have just confirmed that this CL fixes the issue with Chromium + ThinLTO + debug info described here: https://crbug.com/682773#c9

[mehdi_amini]

In D28913#659209, @krasin wrote:

I have just confirmed that this CL fixes the issue with Chromium + ThinLTO + debug info described here: https://crbug.com/682773#c9

This should only be a compile-time optimization for CFI. So this is another issue that should be address separately, and likely first.

[mehdi_amini]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

265 ↗

(On Diff #85015)

Are you expecting this to be involved?

[pcc]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

265 ↗

(On Diff #85015)

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.

[mehdi_amini]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

265 ↗

(On Diff #85015)

Which bot? Chromium bot? Why don't you patch this internally?

[pcc]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

265 ↗

(On Diff #85015)

Which bot? Chromium bot?

Yes.

Why don't you patch this internally?

We don't normally do that.

[krasin]

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

265 ↗

(On Diff #85015)

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.

[pcc]

Ping, D29240 has landed now.

[tejohnson]

This LGTM as a temporary approach - but please wait for Mehdi. Was 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?

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

265 ↗

(On Diff #85015)

Suggest adding a comment about why this is safe, and a FIXME to have CloneModule optionally ignore the debug info.

[pcc]

In D28913#670198, @tejohnson wrote:

This LGTM as a temporary approach - but please wait for Mehdi. Was D29240 the fix of the root cause of the bot failure?

Yes.

Is this fix required to work around a breakage now, or just an optimization to remove unnecessary debug info?

The latter.

(FWIW, I wouldn't necessarily call this a "temporary" approach. I'd say that the benefit of adding the flag to CloneModule should first be proven through benchmarking.)

[mehdi_amini]

I thought after discussing this on IRC this had landed two weeks ago! Apparently I didn't click approve here...

[mehdi_amini]

In D28913#670212, @pcc wrote:

(FWIW, I wouldn't necessarily call this a "temporary" approach. I'd say that the benefit of adding the flag to CloneModule should first be proven through benchmarking.)

One could argue on the opposite. Before you prove that it does not matter, this can't be called anything else than temporary.
Making the software 1% slower here, and here, and here, and here, leads to a bloated project. If we have code reviews it is also to catch what we *know* immediately isn't "right" and not build technical debt.

llvm/trunk/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Analysis/ModuleSummaryAnalysis.h"
 #include "llvm/Analysis/TypeMetadataUtils.h"
 #include "llvm/Bitcode/BitcodeWriter.h"
 #include "llvm/IR/Constants.h"
+#include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 using namespace llvm;
 
auto IsInMergedM = [&](const GlobalValue *GV) {
 
     SmallVector<MDNode *, 1> MDs;
     GVar->getMetadata(LLVMContext::MD_type, MDs);
     return !MDs.empty();
   };
 
   ValueToValueMapTy VMap;
   std::unique_ptr<Module> MergedM(CloneModule(&M, VMap, IsInMergedM));
+  StripDebugInfo(*MergedM);
 
   filterModule(&M, [&](const GlobalValue *GV) { return !IsInMergedM(GV); });
 
   promoteInternals(*MergedM, M, ModuleId);
   promoteInternals(M, *MergedM, ModuleId);
 
   simplifyExternals(*MergedM);
 

llvm/trunk/test/Transforms/ThinLTOBitcodeWriter/split.ll

 ; M0: define i8* @f()
 ; M1-NOT: @f()
 define i8* @f() {
   ret i8* @g
 }
 
 ; M1: !0 = !{i32 0, !"typeid"}
 !0 = !{i32 0, !"typeid"}
+
+; M0: !llvm.dbg.cu
+; M1-NOT: !llvm.dbg.cu
+!llvm.dbg.cu = !{}
+
+!1 = !{i32 2, !"Debug Info Version", i32 3}
+!llvm.module.flags = !{!1}