Retain both sets of debug intrinsics in HoistThenElseCodeToIf (fixes PR 36410)

[uweigand]

Summary

PR 36410 describes a problem where as a result of hoisting common code from "then" and "else" branches of a condition to before the "if", we get a llvm.dbg.value intrinsic with an invalid location. For those intrinsics, the scope indicated in the !dbg location must be identical with the scope of the variable tracked by the intrinsics, or else the module verifier will abort. This constraint can be violated by the current algorithm of getMergedLocation.

Following discussion in https://reviews.llvm.org/D43687, this patch attempts to implement a different approach to fixing this problem. Instead of attempting to merge the locations of two debug intrinsics, we now simply always keep all debug intrinsics from both sides of the "if" during HoistThenElseCodeToIf. This has the additional benefit that we no longer throw away still valid information to help generate better debug data.

[vsk]

This looks like a nice improvement. I think the test could be tighter/simpler though, e.g:

; RUN: opt -debugify -simplifycfg -S < %s | FileCheck %s

define i64 @caller(i64* %ptr, i64 %flag) {
init:
  %v9 = icmp eq i64 %flag, 0
  br i1 %v9, label %a, label %b

; CHECK:  %vala = load i64, i64* %ptr
; CHECK-NEXT:  call void @llvm.dbg.value(metadata i64 %vala
; CHECK-NEXT:  %valbmasked = and i64 %vala, 1
; CHECK-NEXT:  call void @llvm.dbg.value(metadata i64 %vala

a:                                              ; preds = %init
  %vala = load i64, i64* %ptr, align 8
  br label %test.exit

b:                                              ; preds = %init
  %valb = load i64, i64* %ptr, align 8
  %valbmasked = and i64 %valb, 1
  br label %test.exit

test.exit:                                      ; preds = %a, %b
  %retv = phi i64 [ %vala, %a ], [ %valbmasked, %b ]
  ret i64 %retv
}

[uweigand]

Updated test case as requested.

[vsk]

Thanks, lgtm. I tested this out with a stage2 build locally without issue.

[efriedma]

lib/Transforms/Utils/SimplifyCFG.cpp

1261

If I'm following this correctly, you're hoisting all dbg.value intrinsics from both sides to the predecessor. So given code like the following:

int foo(bool b) {
  int a;
  if (b) {
    a = 10;
    bar();
  } else {
    a = 20;
    baz();
  }
  return a;
}

You'll end up with two dbg.value intrinsics in the entry block for the variable "a", so the debugger will choose randomly (?) whether to print a=10 or a=20. That seems wrong.

[uweigand]

Sorry, I didn't see your comment before checking this in. I believe you're right, and this can cause problems --- I've now reverted and will have another look.

[efriedma]

I think you get approximately the right behavior if you just unconditionally ignore the debug info intrinsics (and never hoist them). Of course, the debug info gets dropped if the terminator is hoisted, but that seems okay; it's hard to handle that well in general anyway.

[vsk]

In order for both dbg.values for "a" to be hoisted in Eli's example, wouldn't the stores into "a" need to be identical?

[efriedma]

In order for both dbg.values for "a" to be hoisted in Eli's example, wouldn't the stores into "a" need to be identical?

After mem2reg runs, there is no store instruction, just a PHI in the return block.

[uweigand]

In D44312#1033382, @efriedma wrote:

I think you get approximately the right behavior if you just unconditionally ignore the debug info intrinsics (and never hoist them). Of course, the debug info gets dropped if the terminator is hoisted, but that seems okay; it's hard to handle that well in general anyway.

This is of course an option, but it would break an existing test case (test/Transforms/SimplifyCFG/hoist-dbgvalue.ll) that was added by dpatel in 2011 to specifically verify that the following case is handled: the terminator is hoisted, but both "then" and "else" branch contain a dbg.value statement declaring the same value for the same variable -- in this case, the test case wants to verify that we still retain a dbg.value for that value in that variable.

The problem is that the code that does this hoisting of dbg.value statements for the same value/variable can still generate invalid results due to a mismatching !dbg location metadata field, that's the root cause of PR 36410. So far, a number of different potential solutions have been proposed:

• make sure the merged !dbg location matches the debug value -- this is https://reviews.llvm.org/D43687, but that hasn't been approved yet since it may cause creation of "strange" locations (e.g. using inlined-to fields pointing to places where there is no inlining in the source code)
• do not merge debug values, but hoist them both -- this is the current Phabricator, which has the problem you pointed out
• do not hoist debug values at all -- your latest suggestion, but this would break the existing test case

I'm a bit unsure now what the best way to proceed is at this point. My main goal right now is simply to fix the internal compiler errors in PR 36410 ...

Would it be an option to only hoist debug info instructions where both variable and value match (just like the existing code), but then not attempt to merge the !dbg locations, but instead just hoist both copies of the original instructions with both the original locations (like in this patch)?

[uweigand]

Would it be an option to only hoist debug info instructions where both variable and value match (just like the existing code), but then not attempt to merge the !dbg locations, but instead just hoist both copies of the original instructions with both the original locations (like in this patch)?

Updated the patch to implement the above option, just to have a better basis for discussion ...

[aprantl]

lib/Transforms/Utils/SimplifyCFG.cpp

1316

Maybe formulate this even stronger:
"The debug location is an integral part of a debug info intrinsic and can't be separated from it or replaced."

[uweigand]

Updated comment as requested.

lib/Transforms/Utils/SimplifyCFG.cpp

static bool HoistThenElseCodeToIf(BranchInst *BI,
   // identical order.
   BasicBlock *BB1 = BI->getSuccessor(0); // The true destination.
   BasicBlock *BB2 = BI->getSuccessor(1); // The false destination
 
   BasicBlock::iterator BB1_Itr = BB1->begin();
   BasicBlock::iterator BB2_Itr = BB2->begin();
 
   Instruction *I1 = &*BB1_Itr++, *I2 = &*BB2_Itr++;
-  // Skip debug info if it is not identical.
-  DbgInfoIntrinsic *DBI1 = dyn_cast<DbgInfoIntrinsic>(I1);
-  DbgInfoIntrinsic *DBI2 = dyn_cast<DbgInfoIntrinsic>(I2);
-  if (!DBI1 || !DBI2 || !DBI1->isIdenticalToWhenDefined(DBI2)) {
-    while (isa<DbgInfoIntrinsic>(I1))
+  BasicBlock *BIParent = BI->getParent();
+
+  bool Changed = false;
+  for (;;) {
+    // Move over debug info from both sides unchanged.
+    while (isa<DbgInfoIntrinsic>(I1)) {
+      BIParent->getInstList().splice(BI->getIterator(), BB1->getInstList(), I1);
       I1 = &*BB1_Itr++;
-    while (isa<DbgInfoIntrinsic>(I2))
+    }
+    while (isa<DbgInfoIntrinsic>(I2)) {
+      BIParent->getInstList().splice(BI->getIterator(), BB2->getInstList(), I2);
       I2 = &*BB2_Itr++;
     }
-  if (isa<PHINode>(I1) || !I1->isIdenticalToWhenDefined(I2) ||
-      (isa<InvokeInst>(I1) && !isSafeToHoistInvoke(BB1, BB2, I1, I2)))
-    return false;
 
-  BasicBlock *BIParent = BI->getParent();
+    if (isa<PHINode>(I1) || !I1->isIdenticalToWhenDefined(I2))
+      return Changed;
 
-  bool Changed = false;
-  do {
     // If we are hoisting the terminator instruction, don't move one (making a
     // broken BB), instead clone it, and remove BI.
     if (isa<TerminatorInst>(I1))
       goto HoistTerminator;
 
     // If we're going to hoist a call, make sure that the two instructions we're
     // commoning/hoisting are both marked with musttail, or neither of them is
     // marked as such. Otherwise, we might end up in a situation where we hoist
unsigned KnownIDs[] = {LLVMContext::MD_tbaa,
                            LLVMContext::MD_dereferenceable_or_null,
                            LLVMContext::MD_mem_parallel_loop_access};
     combineMetadata(I1, I2, KnownIDs);
 
     // I1 and I2 are being combined into a single instruction.  Its debug
     // location is the merged locations of the original instructions.
     I1->applyMergedLocation(I1->getDebugLoc(), I2->getDebugLoc());
 
     I2->eraseFromParent();
     Changed = true;
 
     I1 = &*BB1_Itr++;
     I2 = &*BB2_Itr++;
-    // Skip debug info if it is not identical.
-    DbgInfoIntrinsic *DBI1 = dyn_cast<DbgInfoIntrinsic>(I1);
-    DbgInfoIntrinsic *DBI2 = dyn_cast<DbgInfoIntrinsic>(I2);
-    if (!DBI1 || !DBI2 || !DBI1->isIdenticalToWhenDefined(DBI2)) {
-      while (isa<DbgInfoIntrinsic>(I1))
-        I1 = &*BB1_Itr++;
-      while (isa<DbgInfoIntrinsic>(I2))
-        I2 = &*BB2_Itr++;
-    }
-  } while (I1->isIdenticalToWhenDefined(I2));
-
-  return true;
 
 HoistTerminator:
   // It may not be possible to hoist an invoke.
   if (isa<InvokeInst>(I1) && !isSafeToHoistInvoke(BB1, BB2, I1, I2))
     return Changed;
 
   for (BasicBlock *Succ : successors(BB1)) {
     for (PHINode &PN : Succ->phis()) {

test/Transforms/SimplifyCFG/hoist-dbgvalue-inlined.ll

+; RUN: opt -simplifycfg -S < %s | FileCheck %s
+; Verify that we don't crash due an invalid !dbg location on the hoisted llvm.dbg.value
+
+define i64 @caller(i64* %ptr, i64 %flag) !dbg !10 {
+init:
+  %v9 = icmp eq i64 %flag, 0
+  br i1 %v9, label %a, label %b
+
+; CHECK:  %vala = load i64, i64* %ptr
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata i64 %vala, metadata [[MD:![0-9]*]]
+; CHECK-NEXT:  call void @llvm.dbg.value(metadata i64 %vala, metadata [[MD]]
+; CHECK-NEXT:  %valbmasked = and i64 %vala, 1
+
+a:                                              ; preds = %init
+  %vala = load i64, i64* %ptr, align 8
+  call void @llvm.dbg.value(metadata i64 %vala, metadata !8, metadata !DIExpression()), !dbg !12
+  br label %test.exit
+
+b:                                              ; preds = %init
+  %valb = load i64, i64* %ptr, align 8
+  call void @llvm.dbg.value(metadata i64 %valb, metadata !8, metadata !DIExpression()), !dbg !13
+  %valbmasked = and i64 %valb, 1
+  br label %test.exit
+
+test.exit:                                      ; preds = %a, %b
+  %retv = phi i64 [ %vala, %a ], [ %valbmasked, %b ]
+  ret i64 %retv
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+
+attributes #0 = { nounwind readnone speculatable }
+
+!llvm.module.flags = !{!0}
+!llvm.dbg.cu = !{!1}
+
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !2, isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !3)
+!2 = !DIFile(filename: "optbug", directory: "")
+!3 = !{}
+!4 = distinct !DISubprogram(name: "callee", scope: !2, file: !2, line: 1, type: !5, isLocal: false, isDefinition: true, scopeLine: 1, isOptimized: false, unit: !1, variables: !7)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null}
+!7 = !{!8}
+!8 = !DILocalVariable(name: "var", scope: !4, file: !2, type: !9)
+!9 = !DIBasicType(name: "var_t", size: 64, encoding: DW_ATE_unsigned)
+!10 = distinct !DISubprogram(name: "caller", scope: !2, file: !2, line: 5, type: !5, isLocal: false, isDefinition: true, scopeLine: 5, isOptimized: false, unit: !1, variables: !3)
+!11 = distinct !DILocation(line: 6, scope: !10)
+!12 = !DILocation(line: 2, scope: !4, inlinedAt: !11)
+!13 = !DILocation(line: 3, scope: !4, inlinedAt: !11)