[DebugInfo][OPT] Fixing a couple of DI duplication bugs of CloneModule

[rtereshin]

Summary

As demonstrated by the regression tests added in this patch, the
following cases are valid cases:

1. A Function with no DISubprogram attached, but various debug info related to its instructions, coming, for instance, from an inlined function, also defined somewhere else in the same module;
2. ... or coming exclusively from the functions inlined and eliminated from the module entirely.

The ValueMap shared between CloneFunctionInto calls within CloneModule
needs to contain identity mappings for all of the DISubprogram's to
prevent them from being duplicated by MapMetadata / RemapInstruction
calls, this is achieved via DebugInfoFinder collecting all the
DISubprogram's. However, CloneFunctionInto was missing calls into
DebugInfoFinder for functions w/o DISubprogram's attached, but still
referring DISubprogram's from within (case 1). This patch fixes that.

The fix above, however, exposes another issue: if a module contains a
DISubprogram referenced only indirectly from other debug info
metadata, but not attached to any Function defined within the module
(case 2), cloning such a module causes a DICompileUnit duplication: it
will be moved in indirecty via a DISubprogram by DebugInfoFinder first
(because of the first bug fix described above), without being
self-mapped within the shared ValueMap, and then will be copied during
named metadata cloning. So this patch makes sure DebugInfoFinder
visits DICompileUnit's referenced from DISubprogram's as it goes w/o
re-processing llvm.dbg.cu list over and over again for every function
cloned, and makes sure that CloneFunctionInto self-maps
DICompileUnit's referenced from the entire function, not just its own
DISubprogram attached that may also be missing.

The most convenient way of tesing CloneModule I found is to rely on
CloneModule call from opt -run-twice, instead of writing tedious
unit tests. That feature has a couple of properties that makes it hard
to use for this purpose though:

1. CloneModule doesn't copy source filename, making opt -run-twice report it as a difference.
2. opt -run-twice does the second run on the original module, not its clone, making the result of cloning completely invisible in opt's actual output with and without -run-twice both, which directly contradicts opt -run-twices own error message.

This patch fixes this as well.

[rtereshin]

include/llvm/IR/DebugInfo.h

70 ↗

(On Diff #142267)

extracted from processModule and extended to sync with the CloneBasicBlock's version of the events.

85 ↗

(On Diff #142267)

extracted from processModule

91 ↗

(On Diff #142267)

NFC, just put in alphabetical order.

lib/IR/DebugInfo.cpp

75 ↗

(On Diff #142267)

NFC

115 ↗

(On Diff #142267)

extracted from processModule and extended to sync with the CloneBasicBlock's version of the events.

179 ↗

(On Diff #142267)

Fixing case 2 (see the summary).

lib/Transforms/Utils/CloneFunction.cpp

56 ↗

(On Diff #142267)

NFC

171 ↗

(On Diff #142267)

Fixing case 1 (see the summary).

198 ↗

(On Diff #142267)

Fixing case 2 (see the summary).

200 ↗

(On Diff #142267)

NFC

lib/Transforms/Utils/CloneModule.cpp

53 ↗

(On Diff #142267)

Reducing the number of false-failure reports from opt -run-twice

tools/opt/opt.cpp

772 ↗

(On Diff #142267)

Moving from

opt:
  Module -> first run -> visible output

opt -run-twice:
  Module -> CloneModule -> first run -> discarded
        \-> second run -> visible output

scheme to

opt:
  Module -> first run -> visible output

opt -run-twice:
  Module -> first run -> discarded
        \-> CloneModule -> second run -> visible output

to

1. Make sure that opt -run-twice only discards the output of the (full) run that is easily reproducible with a visible output (by just dropping -run-twice in this case)
2. opt could be used to debug CloneModule

782 ↗

(On Diff #142267)

NFC

[aprantl]

Could you split out the NFC changes into a separate commit (no need for a review) and update the diff? It will make reviewing the actual changes much easier. Thanks!

include/llvm/IR/DebugInfo.h

69 ↗

(On Diff #142267)

The \brief is unnecessary, we have autobrief enabled in our Doxyfile.

[vsk]

lib/Transforms/Utils/CloneFunction.cpp

52 ↗

(On Diff #142267)

Might be nice to rewrite this as 'for (Instruction *I : *BB)' while we're changing things.

[rtereshin]

include/llvm/IR/DebugInfo.h

69 ↗

(On Diff #142267)

I'm removing \briefs from the entire header in the second NFC-commit you asked for then, thanks!

lib/Transforms/Utils/CloneFunction.cpp

52 ↗

(On Diff #142267)

Sure, doing that in the follow-up NFC-commit @aprantl asked me for, thanks!

[rtereshin]

The patch is split into 2 commits, FC and a follow-up NFC not included in the review, as requested by @aprantl.
All the comments so far are asking for NFC, addressed in the NFC-commit.

[rtereshin]

lib/IR/DebugInfo.cpp

107 ↗

(On Diff #142303)

Syncing this up with what CloneBasicBlock is doing, extracted as a separate method and re-used in the follow-up NFC commit.

[aprantl]

Basically looks good, a few more comments would be nice (see inline).

lib/IR/DebugInfo.cpp

111 ↗

(On Diff #142303)

Can you add a comment explaining why this is necessary (like what you wrote for the phabricator review)?

137 ↗

(On Diff #142303)

else llvm_unreachable("unexpected imported entity type")?

179 ↗

(On Diff #142267)

Let's just add a comment into the code here explaining why this call is necessary.

test/DebugInfo/X86/clone-module-2.ll

10 ↗

(On Diff #142303)

Can you add a comment explaining what behavior is being tested here?
I.e.: why would running opt invoke the cloner? Is there perhaps a more targeted pass that we could run instead?

tools/opt/opt.cpp

767 ↗

(On Diff #142303)

Ah I see.

[rtereshin]

lib/IR/DebugInfo.cpp

111 ↗

(On Diff #142303)

Sure, will do.

137 ↗

(On Diff #142303)

Good idea, I'll try that.

tools/opt/opt.cpp

767 ↗

(On Diff #142303)

Yeah, I think the fact that -run-twice is invoking cloner is self-evident enough to not elaborate on that, we already have regression tests that rely on it. They aren't very honest, btw, as prior to this patch the opt's output is the original module, not the clone, so their FileCheck's are practically useless. Still works as opt compares the output of the first and second run, so if they differ, it will return with a non-zero return code and the test will still catch it, but anyway that kind of thing is just misleading. I think with these changes it's a tad better as opt will at least output the clone.

As for commenting what tests actually test I think it still needs to be done, thanks.

[rtereshin]

lib/IR/DebugInfo.cpp

111 ↗

(On Diff #142303)

I'm adding the comment to processSubprograms implementation at processCompileUnit's call-site, as the processCompileUnit being a separate method is just an obvious decomposition / DRY thing.

137 ↗

(On Diff #142303)

Yep, passed the tests locally just fine, doing that.

179 ↗

(On Diff #142267)

Done.

test/DebugInfo/X86/clone-module-2.ll

10 ↗

(On Diff #142303)

Done, see below.

tools/opt/opt.cpp

767 ↗

(On Diff #142303)

I've added the comments.

[rtereshin]

Addressing the comments.

[rtereshin]

lib/IR/DebugInfo.cpp

91 ↗

(On Diff #142450)

This is all replaced by a processCompileUnit's call in the follow-up NFC commit, as the pieces are identical.

[rtereshin]

@aprantl Thank you!

[alberto_magni]

llvm/trunk/lib/IR/DebugInfo.cpp

141

This commit is causing failures in my SPEC2006 tests (xalancbmk among others) with -flto=thin and -g.
The unreachable in this line gets triggered.

This is because the if-cascade above does not handle the case for DIGlobalVariable.
Probably the code that handles GlobalVariables at the top of the function needs to be replicated in one if-case here too.

[rtereshin]

llvm/trunk/lib/IR/DebugInfo.cpp

141

Hi @alberto_magni,

Is there a chance you could extract a regression test from one of those SPEC2006 tests?

Thanks!
Roman

[aprantl]

llvm/trunk/lib/IR/DebugInfo.cpp

141

If it's just a DIGlobalVariable appearing in that list, you should be able to construct one manually without much trouble.

[aprantl]

llvm/trunk/lib/IR/DebugInfo.cpp

141

$ cat /tmp/using.cpp
namespace s {
  int i;
}

using s::i;

int f() { return i; }

Using declarations are represented as imported entities.

[aprantl]

llvm/trunk/lib/IR/DebugInfo.cpp

141

That also means that we'll need to accept pretty much anything in that list :-(

[rtereshin]

llvm/trunk/lib/IR/DebugInfo.cpp

141

Thanks, I'll try that!

So, we gotta remove the unreachable, I suppose?

[aprantl]

llvm/trunk/lib/IR/DebugInfo.cpp

141

Yes, but perhaps think about what else might appear in a using statement that we would need to process here and also whether it can be dangerous to process something twice, because it is reachable via the imported entities and some other means (as in my example above) so we don't end up with duplicates / infinite loops / etc.

[rtereshin]

llvm/trunk/lib/IR/DebugInfo.cpp

141

I see.

I don't know off the top of my head how to turn this "global variable as an imported entity" example into a test that would undeniably break one of the DebugInfoFinder users given that llvm_unreachable is removed. DebugInfoFinder tracks types, and CloneFunction self-maps them for some reason, while DIGlobalVariable references a type, so maybe it may cause some debug info duplication if CloneModule is called?

There are also ModuleDebugInfoPrinter and StripDeadDebugInfo module passes, not sure what exactly they expect from DebugInfoFinder.

That being said, if approached meticulously, this is going to take some time.

Do you think we need to remove llvm_unreachable first right now before we do anything else?

[aprantl]

Let's remove the unreachable and add a simple testcase that would trigger it as a first step.

[rtereshin]

Doing that.

[rtereshin]

@aprantl PTAL: https://reviews.llvm.org/D45707

llvm/trunk/include/llvm/IR/DebugInfo.h

public:
   /// \brief Clear all lists.
   void reset();
 
 private:
   void InitializeTypeMap(const Module &M);
 
   void processType(DIType *DT);
   void processSubprogram(DISubprogram *SP);
+  void processCompileUnit(DICompileUnit *CU);
   void processScope(DIScope *Scope);
   bool addCompileUnit(DICompileUnit *CU);
   bool addGlobalVariable(DIGlobalVariableExpression *DIG);
   bool addSubprogram(DISubprogram *SP);
   bool addType(DIType *DT);
   bool addScope(DIScope *Scope);
 
 public:

llvm/trunk/lib/IR/DebugInfo.cpp

for (auto *Import : CU->getImportedEntities()) {
       if (auto *T = dyn_cast<DIType>(Entity))
         processType(T);
       else if (auto *SP = dyn_cast<DISubprogram>(Entity))
         processSubprogram(SP);
       else if (auto *NS = dyn_cast<DINamespace>(Entity))
         processScope(NS->getScope());
       else if (auto *M = dyn_cast<DIModule>(Entity))
         processScope(M->getScope());
+      else
+        llvm_unreachable("unexpected imported entity type");
     }
   }
   for (auto &F : M.functions()) {
     if (auto *SP = cast_or_null<DISubprogram>(F.getSubprogram()))
       processSubprogram(SP);
     // There could be subprograms from inlined functions referenced from
     // instructions only. Walk the function to find them.
     for (const BasicBlock &BB : F) {
       for (const Instruction &I : BB) {
-        if (!I.getDebugLoc())
-          continue;
-        processLocation(M, I.getDebugLoc().get());
-      }
-    }
-  }
+        if (auto *DDI = dyn_cast<DbgDeclareInst>(&I))
+          processDeclare(M, DDI);
+        else if (auto *DVI = dyn_cast<DbgValueInst>(&I))
+          processValue(M, DVI);
+
+        if (auto DbgLoc = I.getDebugLoc())
+          processLocation(M, DbgLoc.get());
+      }
+
+void DebugInfoFinder::processCompileUnit(DICompileUnit *CU) {
+  if (!addCompileUnit(CU))
+    return;
+  for (auto DIG : CU->getGlobalVariables()) {
+    if (!addGlobalVariable(DIG))
+      continue;
+    auto *GV = DIG->getVariable();
+    processScope(GV->getScope());
+    processType(GV->getType().resolve());
+  }
+  for (auto *ET : CU->getEnumTypes())
+    processType(ET);
+  for (auto *RT : CU->getRetainedTypes())
+    if (auto *T = dyn_cast<DIType>(RT))
+      processType(T);
+    else
+      processSubprogram(cast<DISubprogram>(RT));
+  for (auto *Import : CU->getImportedEntities()) {
+    auto *Entity = Import->getEntity().resolve();
+    if (auto *T = dyn_cast<DIType>(Entity))
+      processType(T);
+    else if (auto *SP = dyn_cast<DISubprogram>(Entity))
+      processSubprogram(SP);
+    else if (auto *NS = dyn_cast<DINamespace>(Entity))
+      processScope(NS->getScope());
+    else if (auto *M = dyn_cast<DIModule>(Entity))
+      processScope(M->getScope());
+    else
+      llvm_unreachable("unexpected imported entity type");
+  }
 }
 
 void DebugInfoFinder::processLocation(const Module &M, const DILocation *Loc) {
   if (!Loc)
     return;
   processScope(Loc->getScope());
   processLocation(M, Loc->getInlinedAt());
 }
if (auto *LB = dyn_cast<DILexicalBlockBase>(Scope)) {
     processScope(M->getScope());
   }
 }
 
 void DebugInfoFinder::processSubprogram(DISubprogram *SP) {
   if (!addSubprogram(SP))
     return;
   processScope(SP->getScope().resolve());
+  // Some of the users, e.g. CloneFunctionInto / CloneModule, need to set up a
+  // ValueMap containing identity mappings for all of the DICompileUnit's, not
+  // just DISubprogram's, referenced from anywhere within the Function being
+  // cloned prior to calling MapMetadata / RemapInstruction to avoid their
+  // duplication later as DICompileUnit's are also directly referenced by
+  // llvm.dbg.cu list. Thefore we need to collect DICompileUnit's here as well.
+  // Also, DICompileUnit's may reference DISubprogram's too and therefore need
+  // to be at least looked through.
+  processCompileUnit(SP->getUnit());
   processType(SP->getType());
   for (auto *Element : SP->getTemplateParams()) {
     if (auto *TType = dyn_cast<DITemplateTypeParameter>(Element)) {
       processType(TType->getType().resolve());
     } else if (auto *TVal = dyn_cast<DITemplateValueParameter>(Element)) {
       processType(TVal->getType().resolve());
     }
   }

llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp

#endif
   // recursive functions into themselves.
   //
   for (Function::const_iterator BI = OldFunc->begin(), BE = OldFunc->end();
        BI != BE; ++BI) {
     const BasicBlock &BB = *BI;
 
     // Create a new basic block and copy instructions into it!
     BasicBlock *CBB = CloneBasicBlock(&BB, VMap, NameSuffix, NewFunc, CodeInfo,
-                                      SP ? &DIFinder : nullptr);
+                                      ModuleLevelChanges ? &DIFinder : nullptr);
 
     // Add basic block mapping.
     VMap[&BB] = CBB;
 
     // It is only legal to clone a function if a block address within that
     // function is never referenced outside of the function.  Given that, we
     // want to map block addresses from the old function to block addresses in
     // the clone. (This is different from the generic ValueMapper
#endif
   }
 
   for (DISubprogram *ISP : DIFinder.subprograms()) {
     if (ISP != SP) {
       VMap.MD()[ISP].reset(ISP);
     }
   }
 
+  for (DICompileUnit *CU : DIFinder.compile_units())
+    VMap.MD()[CU].reset(CU);
+
   for (auto *Type : DIFinder.types()) {
     VMap.MD()[Type].reset(Type);
   }
 
   // Loop over all of the instructions in the function, fixing up operand
   // references as we go.  This uses VMap to do all the hard work.
   for (Function::iterator BB =
            cast<BasicBlock>(VMap[&OldFunc->front()])->getIterator(),

llvm/trunk/lib/Transforms/Utils/CloneModule.cpp

 }
 
 std::unique_ptr<Module> llvm::CloneModule(
     const Module &M, ValueToValueMapTy &VMap,
     function_ref<bool(const GlobalValue *)> ShouldCloneDefinition) {
   // First off, we need to create the new module.
   std::unique_ptr<Module> New =
       llvm::make_unique<Module>(M.getModuleIdentifier(), M.getContext());
+  New->setSourceFileName(M.getSourceFileName());
   New->setDataLayout(M.getDataLayout());
   New->setTargetTriple(M.getTargetTriple());
   New->setModuleInlineAsm(M.getModuleInlineAsm());
 
   // Loop over all of the global variables, making corresponding globals in the
   // new module.  Here we add them to the VMap and to the new Module.  We
   // don't worry about attributes or initializers, they will come later.
   //

llvm/trunk/test/DebugInfo/X86/clone-module-2.ll

+; RUN: opt -run-twice -verify -S -o - %s | FileCheck %s
+
+; If a module contains a DISubprogram referenced only indirectly from
+; instruction-level debug info metadata, but not attached to any Function
+; defined within the module, cloning such a module with CloneModule was
+; causing a DICompileUnit duplication: it would be moved in indirecty via a
+; DISubprogram by DebugInfoFinder (making sure DISubprogram's don't get
+; duplicated) first without being explicitly self-mapped within the ValueMap
+; shared among CloneFunctionInto calls, and then it would get copied during
+; named metadata cloning.
+;
+; This is to make sure we don't regress on that.
+
+; Derived from the following C-snippet
+;
+; static int eliminated(int j);
+; __attribute__((nodebug)) int nodebug(int k) { return eliminated(k); }
+; __attribute__((always_inline)) static int eliminated(int j) { return j * 2; }
+;
+; compiled with `clang -O1 -g1 -emit-llvm -S`
+
+; CHECK:     DICompileUnit
+; CHECK-NOT: DICompileUnit
+
+source_filename = "clone-module.c"
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx"
+
+; Function Attrs: norecurse nounwind readnone ssp uwtable
+define i32 @nodebug(i32 %k) local_unnamed_addr #0 {
+entry:
+  %mul.i = shl nsw i32 %k, 1, !dbg !8
+  ret i32 %mul.i
+}
+
+attributes #0 = { norecurse nounwind readnone ssp uwtable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 7.0.0 (https://git.llvm.org/git/clang.git/ 195459d046e795f5952f7d2121e505eeb59c5574) (https://git.llvm.org/git/llvm.git/ e9dc5b5ade57869d1a443c568c6cf556ccf3b7af)", isOptimized: true, runtimeVersion: 0, emissionKind: LineTablesOnly, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "/Volumes/Data/llvm/build/obj")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = !{!"clang version 7.0.0 (https://git.llvm.org/git/clang.git/ 195459d046e795f5952f7d2121e505eeb59c5574) (https://git.llvm.org/git/llvm.git/ e9dc5b5ade57869d1a443c568c6cf556ccf3b7af)"}
+!8 = !DILocation(line: 3, column: 72, scope: !9)
+!9 = distinct !DISubprogram(name: "eliminated", scope: !1, file: !1, line: 3, type: !10, isLocal: true, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !2)
+!10 = !DISubroutineType(types: !2)

llvm/trunk/test/DebugInfo/X86/clone-module.ll

+; RUN: opt -run-twice -verify -S -o - %s | FileCheck %s
+
+; The ValueMap shared between CloneFunctionInto calls within CloneModule needs
+; to contain identity mappings for all of the DISubprogram's to prevent them
+; from being duplicated by MapMetadata / RemapInstruction calls, this is
+; achieved via DebugInfoFinder collecting all the DISubprogram's. However,
+; CloneFunctionInto was missing calls into DebugInfoFinder for functions w/o
+; DISubprogram's attached, but still referring DISubprogram's from within.
+;
+; This is to make sure we don't regress on that.
+
+; Derived from the following C-snippet
+;
+;   int inlined(int j);
+;   __attribute__((nodebug)) int nodebug(int k) { return inlined(k); }
+;   __attribute__((always_inline)) int inlined(int j) { return j * 2; }
+;
+; compiled with `clang -O1 -g3 -emit-llvm -S` by removing
+;
+;   call void @llvm.dbg.value(metadata i32 %k, metadata !8, metadata !DIExpression()), !dbg !14
+;
+; line from @nodebug function.
+
+; The @llvm.dbg.value call is manually removed from @nodebug as not having
+; it there also may cause an incorrect remapping of the call in a case of a
+; regression, not just a duplication of a DISubprogram. Namely, the call's
+; metadata !8 2nd argument and the !dbg !14 debug location may get remapped
+; to reference different copies of the DISubprogram, which is verified by IR
+; Verifier, while having DISubprogram duplicates is not.
+
+; CHECK:     DISubprogram
+; CHECK-NOT: DISubprogram
+
+source_filename = "clone-module.c"
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx"
+
+; Function Attrs: nounwind readnone ssp uwtable
+define i32 @nodebug(i32 %k) local_unnamed_addr #0 {
+entry:
+  %mul.i = shl nsw i32 %k, 1, !dbg !15
+  ret i32 %mul.i
+}
+
+; Function Attrs: alwaysinline nounwind readnone ssp uwtable
+define i32 @inlined(i32 %j) local_unnamed_addr #1 !dbg !9 {
+entry:
+  call void @llvm.dbg.value(metadata i32 %j, metadata !8, metadata !DIExpression()), !dbg !14
+  %mul = shl nsw i32 %j, 1, !dbg !15
+  ret i32 %mul, !dbg !16
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #2
+
+attributes #0 = { nounwind readnone ssp uwtable }
+attributes #1 = { alwaysinline nounwind readnone ssp uwtable }
+attributes #2 = { nounwind readnone speculatable }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6}
+!llvm.ident = !{!7}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 7.0.0 (https://git.llvm.org/git/clang.git/ 195459d046e795f5952f7d2121e505eeb59c5574) (https://git.llvm.org/git/llvm.git/ 69ec7d5667e9928db8435bfbee0da151c85a91c9)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "clone-module.c", directory: "/somewhere")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{i32 7, !"PIC Level", i32 2}
+!7 = !{!"clang version 7.0.0 (https://git.llvm.org/git/clang.git/ 195459d046e795f5952f7d2121e505eeb59c5574) (https://git.llvm.org/git/llvm.git/ 69ec7d5667e9928db8435bfbee0da151c85a91c9)"}
+!8 = !DILocalVariable(name: "j", arg: 1, scope: !9, file: !1, line: 3, type: !12)
+!9 = distinct !DISubprogram(name: "inlined", scope: !1, file: !1, line: 3, type: !10, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !13)
+!10 = !DISubroutineType(types: !11)
+!11 = !{!12, !12}
+!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!13 = !{!8}
+!14 = !DILocation(line: 3, column: 48, scope: !9)
+!15 = !DILocation(line: 3, column: 62, scope: !9)
+!16 = !DILocation(line: 3, column: 53, scope: !9)

llvm/trunk/tools/opt/opt.cpp

if (!NoOutput && !AnalyzeOnly) {
     else
       Passes.add(createBitcodeWriterPass(*OS, PreserveBitcodeUseListOrder,
                                          EmitSummaryIndex, EmitModuleHash));
   }
 
   // Before executing passes, print the final values of the LLVM options.
   cl::PrintOptionValues();
 
-  // If requested, run all passes again with the same pass manager to catch
-  // bugs caused by persistent state in the passes
-  if (RunTwice) {
+  if (!RunTwice) {
+    // Now that we have all of the passes ready, run them.
+    Passes.run(*M);
+  } else {
+    // If requested, run all passes twice with the same pass manager to catch
+    // bugs caused by persistent state in the passes.
     std::unique_ptr<Module> M2(CloneModule(*M));
-    Passes.run(*M2);
+    // Run all passes on the original module first, so the second run processes
+    // the clone to catch CloneModule bugs.
+    Passes.run(*M);
     CompileTwiceBuffer = Buffer;
     Buffer.clear();
-  }
 
-  // Now that we have all of the passes ready, run them.
-  Passes.run(*M);
+    Passes.run(*M2);
 
     // Compare the two outputs and make sure they're the same
-  if (RunTwice) {
     assert(Out);
     if (Buffer.size() != CompileTwiceBuffer.size() ||
         (memcmp(Buffer.data(), CompileTwiceBuffer.data(), Buffer.size()) !=
          0)) {
       errs() << "Running the pass manager twice changed the output.\n"
                 "Writing the result of the second run to the specified output.\n"
                 "To generate the one-run comparison binary, just run without\n"
                 "the compile-twice option\n";