[llvm-branch-commits] [llvm] [KeyInstr][SimplifyCFG] Remap atoms when folding br to common succ into pred (PR #133482)

[Orlando Cazalet-Hyams via llvm-branch-commits]

================
@@ -1129,13 +1130,17 @@ static void cloneInstructionsIntoPredecessorBlockAndUpdateSSAUses(
 
     Instruction *NewBonusInst = BonusInst.clone();
 
-    if (!isa<DbgInfoIntrinsic>(BonusInst) &&
-        PTI->getDebugLoc() != NewBonusInst->getDebugLoc()) {
-      // Unless the instruction has the same !dbg location as the original
-      // branch, drop it. When we fold the bonus instructions we want to make
-      // sure we reset their debug locations in order to avoid stepping on
-      // dead code caused by folding dead branches.
-      NewBonusInst->setDebugLoc(DebugLoc());
+    if (!isa<DbgInfoIntrinsic>(BonusInst)) {
+      if (!NewBonusInst->getDebugLoc().isSameSourceLocation(
+              PTI->getDebugLoc())) {
+        // Unless the instruction has the same !dbg location as the original
+        // branch, drop it. When we fold the bonus instructions we want to make
+        // sure we reset their debug locations in order to avoid stepping on
+        // dead code caused by folding dead branches.
+        NewBonusInst->setDebugLoc(DebugLoc());
+      } else if (const DebugLoc &DL = NewBonusInst->getDebugLoc()) {
+        mapAtomInstance(DL, VMap);
----------------
OCHyams wrote:

I'm not sure I fully understand the questions. Do these answers help (or help to see what I'm missing?)

> Will developers potentially step on the same source location twice?

Do you mean, if we have consecutive instructions with the same source location that are all is_stmt? I think the answer is that it depends on debugger implementation. FWIW I think the SCE debugger doesn't do repeated steps in that case atom. My current view is that it's better to include those is_stmts, and let the consumer decide what to do about it.

(Maybe I'm missing your point?)

> wouldn't this be different when the source-location at PTI potentially becomes key because we've remapped a key instruction we've hoisted to beneath it?

I'm not sure what you mean about PTI's location becoming key? e.g. looking at the test - the new branches in `b` and `c` are only "key" because they're clones of the cond br from `d` (which is already "key").

https://github.com/llvm/llvm-project/pull/133482