Simplify CFG before assigning discriminator.

[danielcdh]

Summary

We need to clean up CFG before assigning discriminator to minimize the impact of optimization on debug info.

[dblaikie]

"minimize the impact of optimization on debug info." - what sort of impact are you referring to?

[danielcdh]

In D19926#421359, @dblaikie wrote:

"minimize the impact of optimization on debug info." - what sort of impact are you referring to?

SampleProfile profile is collected on optimized binary. So at profile annotation step, it's better to have an IR resemble the optimized binary structure. Discriminator is the key to represent the profile, and SimplefyCFG is one major optimization that would change CFG. So if we move discriminator assignment after simplycfg, then at profile used at annotation would be more likely to resemble the optimized binary that is used for profiling.

[dnovillo]

Would it be possible to get a test case where we miss discriminator placement if simplifycfg doesn't run beforehand?

[danielcdh]

This patch works the other way: it reduces # of discriminators
assigned because CFGs are simplified. I don't think we can build such
a testcase unless simplifycfg complicates the CFG.

Thanks,
Dehao

[dnovillo]

OK, sure. What I meant is a testcase that motivates this change.

[danielcdh]

The patch has been reverted because it breaks 96 clang tests (I only
do "make check" before committing).

The problem is that this will add a simplify-cfg pass to each clang
invocation as soon as add-discriminator pass is needed (even at -O0).
This will change the output of many clang tests.

Any suggestions what I should do here?

Thanks,
Dehao

include/llvm/Transforms/Scalar/SimplifyCFG.h

public:
 
   /// \brief Construct a pass with a specific bonus threshold.
   SimplifyCFGPass(int BonusInstThreshold);
 
   /// \brief Run the pass over the function.
   PreservedAnalyses run(Function &F, AnalysisManager<Function> &AM);
 };
 
+struct CFGSimplifyPass : public FunctionPass {
+  static char ID; // Pass identification, replacement for typeid
+  unsigned BonusInstThreshold;
+  std::function<bool(const Function &)> PredicateFtor;
+
+  CFGSimplifyPass(int T = -1,
+                  std::function<bool(const Function &)> Ftor = nullptr);
+  bool runOnFunction(Function &F) override;
+
+  void getAnalysisUsage(AnalysisUsage &AU) const override;
+};
 }
 
 #endif

lib/Transforms/Scalar/SimplifyCFGPass.cpp

PreservedAnalyses SimplifyCFGPass::run(Function &F,
   auto &AC = AM.getResult<AssumptionAnalysis>(F);
 
   if (simplifyFunctionCFG(F, TTI, &AC, BonusInstThreshold))
     return PreservedAnalyses::none();
 
   return PreservedAnalyses::all();
 }
 
-namespace {
-struct CFGSimplifyPass : public FunctionPass {
-  static char ID; // Pass identification, replacement for typeid
-  unsigned BonusInstThreshold;
-  std::function<bool(const Function &)> PredicateFtor;
-
-  CFGSimplifyPass(int T = -1,
-                  std::function<bool(const Function &)> Ftor = nullptr)
-      : FunctionPass(ID), PredicateFtor(Ftor) {
-    BonusInstThreshold = (T == -1) ? UserBonusInstThreshold : unsigned(T);
-    initializeCFGSimplifyPassPass(*PassRegistry::getPassRegistry());
-  }
-  bool runOnFunction(Function &F) override {
-    if (skipFunction(F) || (PredicateFtor && !PredicateFtor(F)))
-      return false;
+CFGSimplifyPass::CFGSimplifyPass(int T,
+            std::function<bool(const Function &)> Ftor)
+
+bool CFGSimplifyPass::runOnFunction(Function &F) {
+  if (PredicateFtor && !PredicateFtor(F))
+    return false;
+
+  if (skipFunction(F))
 
-    AssumptionCache *AC =
-        &getAnalysis<AssumptionCacheTracker>().getAssumptionCache(F);
-    const TargetTransformInfo &TTI =
-        getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
-    return simplifyFunctionCFG(F, TTI, AC, BonusInstThreshold);
-  }
 
-  void getAnalysisUsage(AnalysisUsage &AU) const override {
-    AU.addRequired<AssumptionCacheTracker>();
-    AU.addRequired<TargetTransformInfoWrapperPass>();
-    AU.addPreserved<GlobalsAAWrapperPass>();
-  }
-};
-}
+void CFGSimplifyPass::getAnalysisUsage(AnalysisUsage &AU) const {
 
 char CFGSimplifyPass::ID = 0;
 INITIALIZE_PASS_BEGIN(CFGSimplifyPass, "simplifycfg", "Simplify the CFG", false,
                       false)
 INITIALIZE_PASS_DEPENDENCY(TargetTransformInfoWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(AssumptionCacheTracker)
 INITIALIZE_PASS_END(CFGSimplifyPass, "simplifycfg", "Simplify the CFG", false,
                     false)
 
 // Public interface to the CFGSimplification pass
 FunctionPass *
 llvm::createCFGSimplificationPass(int Threshold,
                                   std::function<bool(const Function &)> Ftor) {
   return new CFGSimplifyPass(Threshold, Ftor);
 }

lib/Transforms/Utils/AddDiscriminators.cpp

 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/Scalar.h"
+#include "llvm/Transforms/Scalar/SimplifyCFG.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "add-discriminators"
 
 namespace {
 struct AddDiscriminators : public FunctionPass {
   static char ID; // Pass identification, replacement for typeid
   AddDiscriminators() : FunctionPass(ID) {
     initializeAddDiscriminatorsPass(*PassRegistry::getPassRegistry());
   }
 
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.addRequired<CFGSimplifyPass>();
+  }
+
   bool runOnFunction(Function &F) override;
 };
 } // end anonymous namespace
 
 char AddDiscriminators::ID = 0;
 INITIALIZE_PASS_BEGIN(AddDiscriminators, "add-discriminators",
                       "Add DWARF path discriminators", false, false)
+INITIALIZE_PASS_DEPENDENCY(CFGSimplifyPass)
 INITIALIZE_PASS_END(AddDiscriminators, "add-discriminators",
                     "Add DWARF path discriminators", false, false)
 
 // Command line option to disable discriminator generation even in the
 // presence of debug information. This is only needed when debugging
 // debug info generation issues.
 static cl::opt<bool> NoDiscriminators(
     "no-discriminators", cl::init(false),

test/Transforms/AddDiscriminators/multiple.ll

entry:
   %x = alloca i32, align 4
   store i32 %i, i32* %i.addr, align 4
   %0 = load i32, i32* %i.addr, align 4, !dbg !10
   %cmp = icmp slt i32 %0, 10, !dbg !10
   br i1 %cmp, label %if.then, label %if.else, !dbg !10
 
 if.then:                                          ; preds = %entry
   %1 = load i32, i32* %i.addr, align 4, !dbg !10
-; CHECK:  %1 = load i32, i32* %i.addr, align 4, !dbg ![[THEN:[0-9]+]]
 
   store i32 %1, i32* %x, align 4, !dbg !10
-; CHECK:  store i32 %1, i32* %x, align 4, !dbg ![[THEN]]
+; CHECK:  store i32 %1, i32* %x, align 4, !dbg ![[THEN:[0-9]+]]
 
   br label %if.end, !dbg !10
 ; CHECK:  br label %if.end, !dbg ![[THEN]]
 
 if.else:                                          ; preds = %entry
   %2 = load i32, i32* %i.addr, align 4, !dbg !10
-; CHECK:  %2 = load i32, i32* %i.addr, align 4, !dbg ![[ELSE:[0-9]+]]
 
   %sub = sub nsw i32 0, %2, !dbg !10
-; CHECK:  %sub = sub nsw i32 0, %2, !dbg ![[ELSE]]
+; CHECK:  %sub = sub nsw i32 0, %1, !dbg ![[ELSE:[0-9]+]]
 
   store i32 %sub, i32* %x, align 4, !dbg !10
 ; CHECK:  store i32 %sub, i32* %x, align 4, !dbg ![[ELSE]]
 
   br label %if.end
 
 if.end:                                           ; preds = %if.else, %if.then
   ret void, !dbg !12

test/Transforms/AddDiscriminators/oneline.ll

define i32 @_Z3fooi(i32 %i) #0 !dbg !4 {
   store i32 %i, i32* %2, align 4, !tbaa !13
   call void @llvm.dbg.declare(metadata i32* %2, metadata !9, metadata !17), !dbg !18
   %3 = load i32, i32* %2, align 4, !dbg !19, !tbaa !13
   %4 = icmp eq i32 %3, 3, !dbg !21
   br i1 %4, label %8, label %5, !dbg !22
 
 ; <label>:5                                       ; preds = %0
   %6 = load i32, i32* %2, align 4, !dbg !23, !tbaa !13
-; CHECK:  %6 = load i32, i32* %2, align 4, !dbg ![[THEN1:[0-9]+]],{{.*}}
-
   %7 = icmp eq i32 %6, 5, !dbg !24
-; CHECK:  %7 = icmp eq i32 %6, 5, !dbg ![[THEN2:[0-9]+]]
-
   br i1 %7, label %8, label %9, !dbg !25
-; CHECK:  br i1 %7, label %8, label %9, !dbg ![[THEN3:[0-9]+]]
 
 ; <label>:8                                       ; preds = %5, %0
   store i32 100, i32* %1, align 4, !dbg !26
-; CHECK: store i32 100, i32* %1, align 4, !dbg ![[ELSE:[0-9]+]]
+; CHECK: store i32 100, i32* %1, align 4, !dbg ![[THEN:[0-9]+]]
 
   br label %10, !dbg !26
-; CHECK: br label %10, !dbg ![[ELSE]]
 
 ; <label>:9                                       ; preds = %5
   store i32 99, i32* %1, align 4, !dbg !27
-; CHECK: store i32 99, i32* %1, align 4, !dbg ![[COMBINE:[0-9]+]]
+; CHECK: store i32 99, i32* %1, align 4, !dbg ![[ELSE:[0-9]+]]
 
   br label %10, !dbg !27
-; CHECK: br label %10, !dbg ![[COMBINE]]
 
 ; <label>:10                                      ; preds = %9, %8
   %11 = load i32, i32* %1, align 4, !dbg !28
   ret i32 %11, !dbg !28
 }
 
 ; Function Attrs: nounwind readnone
 declare void @llvm.dbg.declare(metadata, metadata, metadata) #1

 !22 = !DILocation(line: 2, column: 14, scope: !20)
 !23 = !DILocation(line: 2, column: 17, scope: !20)
 !24 = !DILocation(line: 2, column: 19, scope: !20)
 !25 = !DILocation(line: 2, column: 7, scope: !4)
 !26 = !DILocation(line: 2, column: 25, scope: !20)
 !27 = !DILocation(line: 2, column: 42, scope: !20)
 !28 = !DILocation(line: 3, column: 1, scope: !4)
 
-; CHECK: ![[THEN1]] = !DILocation(line: 2, column: 17, scope: ![[THENBLOCK:[0-9]+]])
+; CHECK: ![[THEN]] = !DILocation(line: 2, column: 25, scope: ![[THENBLOCK:[0-9]+]])
 ; CHECK: ![[THENBLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 1)
-; CHECK: ![[THEN2]] = !DILocation(line: 2, column: 19, scope: ![[THENBLOCK]])
-; CHECK: ![[THEN3]] = !DILocation(line: 2, column: 7, scope: ![[THENBLOCK]])
-; CHECK: ![[ELSE]] = !DILocation(line: 2, column: 25, scope: ![[ELSEBLOCK:[0-9]+]])
+; CHECK: ![[ELSE]] = !DILocation(line: 2, column: 42, scope: ![[ELSEBLOCK:[0-9]+]])
 ; CHECK: ![[ELSEBLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 2)
-; CHECK: ![[COMBINE]] = !DILocation(line: 2, column: 42, scope: ![[COMBINEBLOCK:[0-9]+]])
-; CHECK: ![[COMBINEBLOCK]] = !DILexicalBlockFile({{.*}} discriminator: 3)