[BranchFolding] Remove getBranchDebugLoc()

[ellishg]

getBranchDebugLoc() was added 12 years ago in 7c5dcb6, before MachineBasicBlock::findBranchDebugLoc() was created 06a2128 7 years ago. These two functions do almost the same thing, except the later merges debug info from all terminators.

[jmorse]

This makes sense to me; @SLTozer has been thinking about branch-source-loc merging recently, there aren't any concerns with more parts of LLVM merging branch-source-locs together are there?

This may seem nitpicky, but I'd much prefer using explicit DebugLoc rather than auto for this code: I don't believe there's any ergonomic benefit from using auto here aside from fewer characters on some already small lines, and it un-necessarily obscures what's going on. Specifically, those who aren't familiar with the LLVM code base will find this harder to deal with, in exchange for very little benefit overall. (IMHO, YMMV)

[ellishg]

This makes sense to me; @SLTozer has been thinking about branch-source-loc merging recently, there aren't any concerns with more parts of LLVM merging branch-source-locs together are there?

I actually removed this code in MachineBasicBlock::findBranchDebugLoc() and I didn't see a single test failure. So I don't think it is common for two terminators in the same MBB to have different debug locs.

llvm-project/llvm/lib/CodeGen/MachineBasicBlock.cpp

Lines 1565 to 1567 in 012dd8b

	for (++TI ; TI != end() ; ++TI)
	if (TI->isBranch())
	DL = DILocation::getMergedLocation(DL, TI->getDebugLoc());

I also looked at the implementation of DILocation::getMergedLocation() and realized that it will clear the line/column of merged locs, which is probably not what we want.

llvm-project/llvm/lib/IR/DebugInfoMetadata.cpp

Lines 212 to 217 in e394fec

	bool SameLine = L1->getLine() == L2->getLine();
	bool SameCol = L1->getColumn() == L2->getColumn();
	unsigned Line = SameLine ? L1->getLine() : 0;
	unsigned Col = SameLine && SameCol ? L1->getColumn() : 0;
	return DILocation::get(C, Line, Col, Scope, InlinedAt);

And anyway, I can't find a real-world example that this PR would change. I'm thinking of adding a new parameter like MergeLocs to findBranchDebugLoc() to preserve the behavior of simply grabbing the last debug loc instead of merging. What do you think?

[llvmbot]

@llvm/pr-subscribers-debuginfo

Author: Ellis Hoag (ellishg)

Changes

getBranchDebugLoc() was added 12 years ago in 7c5dcb6, before MachineBasicBlock::findBranchDebugLoc() was created 06a2128 7 years ago. These two functions do almost the same thing, except the later merges debug info from all terminators.

---

Full diff: https://github.com/llvm/llvm-project/pull/114613.diff

1 Files Affected:

• (modified) llvm/lib/CodeGen/BranchFolding.cpp (+17-25)

diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index f8de13650680a8..ae323f1d8faf19 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -1266,15 +1266,6 @@ static bool IsBetterFallthrough(MachineBasicBlock *MBB1,
   return MBB2I->isCall() && !MBB1I->isCall();
 }
 
-/// getBranchDebugLoc - Find and return, if any, the DebugLoc of the branch
-/// instructions on the block.
-static DebugLoc getBranchDebugLoc(MachineBasicBlock &MBB) {
-  MachineBasicBlock::iterator I = MBB.getLastNonDebugInstr();
-  if (I != MBB.end() && I->isBranch())
-    return I->getDebugLoc();
-  return DebugLoc();
-}
-
 static void copyDebugInfoToPredecessor(const TargetInstrInfo *TII,
                                        MachineBasicBlock &MBB,
                                        MachineBasicBlock &PredMBB) {
@@ -1401,11 +1392,11 @@ bool BranchFolder::OptimizeBlock(MachineBasicBlock *MBB) {
     // destination, remove the branch, replacing it with an unconditional one or
     // a fall-through.
     if (PriorTBB && PriorTBB == PriorFBB) {
-      DebugLoc dl = getBranchDebugLoc(PrevBB);
+      DebugLoc Dl = PrevBB.findBranchDebugLoc();
       TII->removeBranch(PrevBB);
       PriorCond.clear();
       if (PriorTBB != MBB)
-        TII->insertBranch(PrevBB, PriorTBB, nullptr, PriorCond, dl);
+        TII->insertBranch(PrevBB, PriorTBB, nullptr, PriorCond, Dl);
       MadeChange = true;
       ++NumBranchOpts;
       goto ReoptimizeBlock;
@@ -1459,9 +1450,9 @@ bool BranchFolder::OptimizeBlock(MachineBasicBlock *MBB) {
     // If the prior block branches somewhere else on the condition and here if
     // the condition is false, remove the uncond second branch.
     if (PriorFBB == MBB) {
-      DebugLoc dl = getBranchDebugLoc(PrevBB);
+      DebugLoc Dl = PrevBB.findBranchDebugLoc();
       TII->removeBranch(PrevBB);
-      TII->insertBranch(PrevBB, PriorTBB, nullptr, PriorCond, dl);
+      TII->insertBranch(PrevBB, PriorTBB, nullptr, PriorCond, Dl);
       MadeChange = true;
       ++NumBranchOpts;
       goto ReoptimizeBlock;
@@ -1473,9 +1464,9 @@ bool BranchFolder::OptimizeBlock(MachineBasicBlock *MBB) {
     if (PriorTBB == MBB) {
       SmallVector<MachineOperand, 4> NewPriorCond(PriorCond);
       if (!TII->reverseBranchCondition(NewPriorCond)) {
-        DebugLoc dl = getBranchDebugLoc(PrevBB);
+        DebugLoc Dl = PrevBB.findBranchDebugLoc();
         TII->removeBranch(PrevBB);
-        TII->insertBranch(PrevBB, PriorFBB, nullptr, NewPriorCond, dl);
+        TII->insertBranch(PrevBB, PriorFBB, nullptr, NewPriorCond, Dl);
         MadeChange = true;
         ++NumBranchOpts;
         goto ReoptimizeBlock;
@@ -1511,9 +1502,9 @@ bool BranchFolder::OptimizeBlock(MachineBasicBlock *MBB) {
           LLVM_DEBUG(dbgs() << "\nMoving MBB: " << *MBB
                             << "To make fallthrough to: " << *PriorTBB << "\n");
 
-          DebugLoc dl = getBranchDebugLoc(PrevBB);
+          DebugLoc Dl = PrevBB.findBranchDebugLoc();
           TII->removeBranch(PrevBB);
-          TII->insertBranch(PrevBB, MBB, nullptr, NewPriorCond, dl);
+          TII->insertBranch(PrevBB, MBB, nullptr, NewPriorCond, Dl);
 
           // Move this block to the end of the function.
           MBB->moveAfter(&MF.back());
@@ -1574,9 +1565,9 @@ bool BranchFolder::OptimizeBlock(MachineBasicBlock *MBB) {
     if (CurTBB && CurFBB && CurFBB == MBB && CurTBB != MBB) {
       SmallVector<MachineOperand, 4> NewCond(CurCond);
       if (!TII->reverseBranchCondition(NewCond)) {
-        DebugLoc dl = getBranchDebugLoc(*MBB);
+        DebugLoc Dl = MBB->findBranchDebugLoc();
         TII->removeBranch(*MBB);
-        TII->insertBranch(*MBB, CurFBB, CurTBB, NewCond, dl);
+        TII->insertBranch(*MBB, CurFBB, CurTBB, NewCond, Dl);
         MadeChange = true;
         ++NumBranchOpts;
         goto ReoptimizeBlock;
@@ -1588,7 +1579,7 @@ bool BranchFolder::OptimizeBlock(MachineBasicBlock *MBB) {
     if (CurTBB && CurCond.empty() && !CurFBB &&
         IsBranchOnlyBlock(MBB) && CurTBB != MBB &&
         !MBB->hasAddressTaken() && !MBB->isEHPad()) {
-      DebugLoc dl = getBranchDebugLoc(*MBB);
+      DebugLoc Dl = MBB->findBranchDebugLoc();
       // This block may contain just an unconditional branch.  Because there can
       // be 'non-branch terminators' in the block, try removing the branch and
       // then seeing if the block is empty.
@@ -1622,9 +1613,9 @@ bool BranchFolder::OptimizeBlock(MachineBasicBlock *MBB) {
               assert(!PriorFBB && "Machine CFG out of date!");
               PriorFBB = MBB;
             }
-            DebugLoc pdl = getBranchDebugLoc(PrevBB);
+            DebugLoc PrevDl = PrevBB.findBranchDebugLoc();
             TII->removeBranch(PrevBB);
-            TII->insertBranch(PrevBB, PriorTBB, PriorFBB, PriorCond, pdl);
+            TII->insertBranch(PrevBB, PriorTBB, PriorFBB, PriorCond, PrevDl);
           }
 
           // Iterate through all the predecessors, revectoring each in-turn.
@@ -1657,10 +1648,11 @@ bool BranchFolder::OptimizeBlock(MachineBasicBlock *MBB) {
               bool NewCurUnAnalyzable = TII->analyzeBranch(
                   *PMBB, NewCurTBB, NewCurFBB, NewCurCond, true);
               if (!NewCurUnAnalyzable && NewCurTBB && NewCurTBB == NewCurFBB) {
-                DebugLoc pdl = getBranchDebugLoc(*PMBB);
+                DebugLoc PrevDl = PMBB->findBranchDebugLoc();
                 TII->removeBranch(*PMBB);
                 NewCurCond.clear();
-                TII->insertBranch(*PMBB, NewCurTBB, nullptr, NewCurCond, pdl);
+                TII->insertBranch(*PMBB, NewCurTBB, nullptr, NewCurCond,
+                                  PrevDl);
                 MadeChange = true;
                 ++NumBranchOpts;
               }
@@ -1679,7 +1671,7 @@ bool BranchFolder::OptimizeBlock(MachineBasicBlock *MBB) {
       }
 
       // Add the branch back if the block is more than just an uncond branch.
-      TII->insertBranch(*MBB, CurTBB, nullptr, CurCond, dl);
+      TII->insertBranch(*MBB, CurTBB, nullptr, CurCond, Dl);
     }
   }
 

[jmorse]

(end-of-year holidays and so forth knocked me out of kilter, sorry for the delay),

I also looked at the implementation of DILocation::getMergedLocation() and realized that it will clear the line/column of merged locs, which is probably not what we want.

It's desirable in certain circumstances, if there was genuine ambiguity between line numbers at the end of a block, it's better for the debugger to say nothing (line zero) than to say something false.

Overall it seems highly unlikely that such a situation would ever occur; and if it did then the merging would make it safe, and that'd be an improvement on what BranchFolding does today. Thus this is all desirable.

I'm adding the debug-info label as this didn't get it before so that it appears on other debug-info folks' radars. If there are no additional concerns from other folks in the next week or so, then LGTM.

[SLTozer]

Just for the extra context, I recently did some hunting to find a case where two terminators in a block had different locations, and I also couldn't find any such cases - though I was looking mainly at x86, I did also try to search for any cases where they would be generated for other targets. I agree with @jmorse that if the DebugLocs were different, and we selected one of those DebugLocs arbitrarily (e.g. whichever comes last), we'd sometimes generate incorrect debug info; I'd say using the merged location is the best bet given the assumption that the locs will almost always (and probably actually always) be the same.

If we were concerned about dropping information, I'd note that in almost all the calls to getBranchDebugLoc() we're really modifying a specific known branch instruction, so could use that known branch's DebugLoc instead of an MBB method.

[ellishg]

The motivation behind this PR was to remove redundant code. If it's accepted I'll land, but if anyone has a problem with it I don't mind closing this PR. From the responses it seems like this PR might be ok, but let me know.

[jmorse]

So, this LGTM for both code consolidation, and potential for new new line-zero behaviour is good. Please wait another day before landing though, just in case others chip in.