[PATCH] D93662: [SimplifyCFG] Keep !dgb metadata of moved instruction, if they match.

Thu Jan 7 07:45:19 PST 2021

fhahn updated this revision to Diff 315142.
fhahn added a comment.



In D93662#2479778 <https://reviews.llvm.org/D93662#2479778>, @vsk wrote:

> In D93662#2479383 <https://reviews.llvm.org/D93662#2479383>, @jmorse wrote:
>
>> In terms of wording, how about adding an additional clause:
>>
>>   A transformation should preserve the debug location of an instruction [...] if the destination block already contains an instruction with an identical debug location
>>
>> Which is slightly broader than your wording @fhahn, but IMHO gets across the idea that the location shouldn't be preserved if it's "new" to a block. Opinions?
>
> I think that sounds reasonable. My initial thought was that the broader language might invite folks to consider scanning the destination block for a matching location: there's nothing wrong with doing that in principle, but imho it ought to be discouraged for performance reasons.

I updated the doc with the suggested wording. Not sure if it is worth adding a clause saying to should only be done when it can be cheaply determined that the location is not new to the block?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93662/new/

https://reviews.llvm.org/D93662

Files:
  llvm/docs/HowToUpdateDebugInfo.rst
  llvm/lib/Transforms/Utils/SimplifyCFG.cpp
  llvm/test/Transforms/SimplifyCFG/fold-debug-location.ll


Index: llvm/test/Transforms/SimplifyCFG/fold-debug-location.ll
===================================================================

--- llvm/test/Transforms/SimplifyCFG/fold-debug-location.ll
+++ llvm/test/Transforms/SimplifyCFG/fold-debug-location.ll
@@ -1,6 +1,7 @@
 ; RUN: opt -S -simplifycfg -simplifycfg-require-and-preserve-domtree=1 < %s | FileCheck %s --match-full-lines
 
 ; Make sure we reset the debug location when folding instructions.
+; CHECK-LABEL: define {{.+}} @patatino({{.+}}
 ; CHECK: [[VAL:%.*]] = and i32 %c2, %k
 ; CHECK-NEXT: [[VAL2:%.*]] icmp eq i32 [[VAL]], 0
 
@@ -24,6 +25,37 @@
   ret i32 undef, !dbg !16
 }
 
+
+; All instructions involved in folding have the same !dbg location. Make sure
+; they are preserved.
+define void @dbg_all_equal(i32 %k, i32 %c1, i32 %c2) !dbg !17 {
+; CHECK-LABEL: define {{.+}} @dbg_all_equal({{.+}}
+; CHECK-NEXT:    [[A1:%[a-z0-9]+]] = and i32 %c1, %k, !dbg [[DBG:![0-9]+]]
+; CHECK-NEXT:    [[C1:%[a-z0-9]+]] = icmp eq i32 [[A1]], 0, !dbg [[DBG]]
+; CHECK-NEXT:    [[A2:%[a-z0-9]+]] = and i32 %c2, %k, !dbg [[DBG]]
+; CHECK-NEXT:    [[C2:%[a-z0-9]+]] = icmp eq i32 [[A2]], 0, !dbg [[DBG]]
+; CHECK-NEXT:    [[OR:%[.a-z0-9]+]] = or i1 [[C1]], [[C2]], !dbg [[DBG]]
+; CHECK-NEXT:    br i1 [[OR]], label {{.+}}, label {{.+}}, !dbg [[DBG]]
+;
+  %1 = and i32 %c1, %k, !dbg !18
+  %2 = icmp eq i32 %1, 0, !dbg !18
+  br i1 %2, label %8, label %3, !dbg !18
+
+3:
+  %4 = and i32 %c2, %k, !dbg !18
+  %5 = icmp eq i32 %4, 0, !dbg !18
+  br i1 %5, label %8, label %6, !dbg !18
+
+6:
+  %7 = tail call i32 (...) @bar(), !dbg !18
+  br label %8, !dbg !18
+
+8:
+  ret void
+}
+
+
+
 !llvm.dbg.cu = !{!0}
 !llvm.debugify = !{!3, !4}
 !llvm.module.flags = !{!5}
@@ -45,3 +77,5 @@
 !14 = !DILocation(line: 7, column: 1, scope: !6)
 !15 = !DILocation(line: 8, column: 1, scope: !6)
 !16 = !DILocation(line: 9, column: 1, scope: !6)
+!17 = distinct !DISubprogram(name: "dbg_all_equal", linkageName: "dbg_all_equal", scope: null, file: !1, line: 1, type: !7, scopeLine: 1, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!18 = !DILocation(line: 10, column: 10, scope: !17)
Index: llvm/lib/Transforms/Utils/SimplifyCFG.cpp
===================================================================
--- llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -3038,10 +3038,13 @@
       if (&BonusInst == Cond)
         CondInPred = NewBonusInst;
 
-      // 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 (PBI->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());
+      }
 
       RemapInstruction(NewBonusInst, VMap,
                        RF_NoModuleLevelChanges | RF_IgnoreMissingLocals);
Index: llvm/docs/HowToUpdateDebugInfo.rst
===================================================================
--- llvm/docs/HowToUpdateDebugInfo.rst
+++ llvm/docs/HowToUpdateDebugInfo.rst
@@ -59,6 +59,17 @@
 * LICM. E.g., if an instruction is moved from the loop body to the preheader,
   the rule for :ref:`dropping locations<WhenToDropLocation>` applies.
 
+In addition to the rule above, a transformation should also preserve the debug
+location of an instruction that is moved between basic blocks, if the
+destination block already contains an instruction with an identical debug
+location.
+
+Examples of transformations that should follow this rule include:
+
+* Moving instructions between basic blocks. For example, if instruction ``I1``
+  in ``BB1`` is moved before ``I2`` in ``BB2``, the source location of ``I1``
+  can be preserved if it has the same source location as ``I2``.
+
 .. _WhenToMergeLocation:
 
 When to merge instruction locations


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D93662.315142.patch
Type: text/x-patch
Size: 4183 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210107/2bd0d730/attachment.bin>
