[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:

> For the first part I think that answers the question (multiple instructions with the same source-location getting is_stmt). Would there also be potential for an intervening non-is_stmt instruction with the same source line between the two? It feels possible from instruction scheduling if not immediately in this pass, and if so I guess it's a general consequence of the technique.

Yeah I think so. And again, kind of up to the debugger how to handle that IMO. FWIW I'm pretty sure our debugger skips forward until the next is_stmt line doesn't have the same line number.

> I was wondering whether there can be a case where the instruction at PTI and the bonus instruction are in the same group, but the one being remapped here has the higher rank, and putting it in a different group causes the previously-lower-ranked instruction to become the highest ranked as a result.

Ah I see what you're saying. Yep, this is possibly edge-case and a subtle limitation of remapping in general. I figured that because it's a bit of a corner case and at worst we see an extra step (not regressing from current behaviour, just an extra step than key instructions might *ideally* want to produce), it wasn't worth worrying about.

> At face value this isn't a problem because the whole point of this function is we're duplicating a code path; but could there be scenarios where LLVM uses this utility to implement a move (i.e. clone-then-delete-old-path?)

I guess in theory... in practice it's only reached through `foldBranchToCommonDest`. I'm not sure if there's any way to detect/prevent future user error here.

> Or to put it another way: I believe (but may be wrong) that cloneInstructionsIntoPredecessorBlockAndUpdateSSAUses currently copies/moves instructions from one place to another. But, with this change, what was a plain copy/move now comes with changes to the stepping behaviour.

It copies rather than moves (because there can be multiple preds that we copy into). However, if there's only ever one pred then yes, this essentially moves-by-copy, and updates atom groups (even though it doesn't really need to). We could say "if there's one pred, then don't remap the atoms". I tested that and it works. But with multiple preds this function gets called multiple times, and in the final call there's only one pred. So that final pred doesn't get remapped atoms. It's a subtle difference, but in terms of "correctness" I think that's ok (it's as-ok as the existing situation).

> I think this boils down to saying "so what?", and the answer to that is "the stepping is still superior to without key instructions". I'm just trying to think through the consequences of how these changes compose together.

I'm leaning towards "so what" and leaving it as I have in this commit. But if my response above sounds better to you, I'll happily make that change.

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