Add control over debug lines dropping

[jacek-galazka]

Summary

Control if GVN and LICM pass should retain debug locations of moved instructions.

[probinson]

No test?

I'm curious what motivated this. I'm sure there are more places that do this kind of thing.

llvm/lib/Transforms/Scalar/LICM.cpp

138

s/lines/line/

[jacek-galazka]

These changes are based on D60913. I can search if there are more DL drops on Monday.
Do you think duplicating and adjusting a test like DebugInfo/Generic/licm-hoist-debug-loc.ll is sufficient?

llvm/lib/Transforms/Scalar/LICM.cpp

138

What do you mean by that?

[dblaikie]

Adding a flag for this seems worthy of a broader design discussion - so far we've been able to align the needs of profilers (never wanting to attribute a sample to a location that didn't actually execute - so hoisted or merged code cannot use one of its original locations) and debugger users. If that alignment is no longer going to be sufficient - I think an llvm-dev discussion about what the goals/priorities/policies are going forward would be good.

(also followed up on the previous review/commit - I've got general doubts about that patch too, that this one would not address)

llvm/lib/Transforms/Scalar/GVN.cpp

 
 // This is based on IsValueFullyAvailableInBlockNumSpeculationsMax stat.
 static cl::opt<uint32_t> MaxBBSpeculations(
     "gvn-max-block-speculations", cl::Hidden, cl::init(600), cl::ZeroOrMore,
     cl::desc("Max number of blocks we're willing to speculate on (and recurse "
              "into) when deducing if a value is fully avaliable or not in GVN "
              "(default = 600)"));
 
+extern cl::opt<bool> AllowJumpyDebugTables;
+
 struct llvm::GVN::Expression {
   uint32_t opcode;
   bool commutative = false;
   Type *type = nullptr;
   SmallVector<uint32_t, 4> varargs;
 
   Expression(uint32_t o = ~2U) : opcode(o) {}
 
bool GVN::PerformLoadPRE(LoadInst *LI, AvailValInBlkVect &ValuesPerBlock,
   LLVM_DEBUG(dbgs() << "GVN REMOVING PRE LOAD: " << *LI << '\n');
   LLVM_DEBUG(if (!NewInsts.empty()) dbgs()
              << "INSERTED " << NewInsts.size() << " INSTS: " << *NewInsts.back()
              << '\n');
 
   // Assign value numbers to the new instructions.
   for (Instruction *I : NewInsts) {
     // Instructions that have been inserted in predecessor(s) to materialize
-    // the load address do not retain their original debug locations. Doing
-    // so could lead to confusing (but correct) source attributions.
+    // the load address can retain their original debug locations. Doing
+    // so leads to confusing source attributions. However as locations are
+    // correct, they can be useful.
     if (const DebugLoc &DL = I->getDebugLoc())
+      if (!AllowJumpyDebugTables)
         I->setDebugLoc(DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));
 
     // FIXME: We really _ought_ to insert these value numbers into their
     // parent's availability map.  However, in doing so, we risk getting into
     // ordering issues.  If a block hasn't been processed yet, we would be
     // marking a value as AVAIL-IN, which isn't what we intend.
     VN.lookupOrAdd(I);
   }
 

llvm/lib/Transforms/Scalar/LICM.cpp

 // compile time.
 cl::opt<unsigned> llvm::SetLicmMssaNoAccForPromotionCap(
     "licm-mssa-max-acc-promotion", cl::init(250), cl::Hidden,
     cl::desc("[LICM & MemorySSA] When MSSA in LICM is disabled, this has no "
              "effect. When MSSA in LICM is enabled, then this is the maximum "
              "number of accesses allowed to be present in a loop in order to "
              "enable memory promotion."));
 
+cl::opt<bool> AllowJumpyDebugTables(
+    "allow-jumpy-debug-lines-tables", cl::Hidden, cl::init(false),
+    cl::desc("Retain debug locations of instructions moved by eg. LICM."));
+
 static bool inSubLoop(BasicBlock *BB, Loop *CurLoop, LoopInfo *LI);
 static bool isNotUsedOrFreeInLoop(const Instruction &I, const Loop *CurLoop,
                                   const LoopSafetyInfo *SafetyInfo,
                                   TargetTransformInfo *TTI, bool &FreeInLoop);
 static void hoist(Instruction &I, const DominatorTree *DT, const Loop *CurLoop,
                   BasicBlock *Dest, ICFLoopSafetyInfo *SafetyInfo,
                   MemorySSAUpdater *MSSAU, ScalarEvolution *SE,
                   OptimizationRemarkEmitter *ORE);
static void hoist(Instruction &I, const DominatorTree *DT, const Loop *CurLoop,
   if (isa<PHINode>(I))
     // Move the new node to the end of the phi list in the destination block.
     moveInstructionBefore(I, *Dest->getFirstNonPHI(), *SafetyInfo, MSSAU, SE);
   else
     // Move the new node to the destination block, before its terminator.
     moveInstructionBefore(I, *Dest->getTerminator(), *SafetyInfo, MSSAU, SE);
 
   // Apply line 0 debug locations when we are moving instructions to different
-  // basic blocks because we want to avoid jumpy line tables.
+  // basic blocks if we want to avoid jumpy line tables.
   if (const DebugLoc &DL = I.getDebugLoc())
+    if (!AllowJumpyDebugTables)
       I.setDebugLoc(DebugLoc::get(0, 0, DL.getScope(), DL.getInlinedAt()));
 
   if (isa<LoadInst>(I))
     ++NumMovedLoads;
   else if (isa<CallInst>(I))
     ++NumMovedCalls;
   ++NumHoisted;
 }