[DebugInfo][Dexter] Unreachable line stepped onto after SimplifyCFG.

[CarlosAlbertoEnciso]

Summary

For the below test:

int main() {
  volatile int foo = 0;
  int read1 = foo;
  int brains = foo;

  if (read1 > 3) {
    brains *= 2;
    brains += 1;
  }

  return brains;
}

When in the debugger, on the line "if (read1 > 3)", and we step from the
'if' condition, onto the addition, then back to the 'if' again, which is
misleading, as described by:

PR38762

[aprantl]

The code looks fine, I have some comments about the comments.

lib/Transforms/Utils/Local.cpp

2535

Please also explain why if possible. I think the reason is different for DILoctions and dbg.values.

2538

TODO: Extend llvm.dbg.value to take more than one SSA Value (PR39141) to encode predicated DIExpressions that yield different results on different code paths.

2540

this sentence is garbled :-)

If I parsed the sentence correctly it sounds like a non-sequitur to me: having conditional dbg.values won't affect our options for DILocations at all. Or are you using "location" in the .debug_loc sense? That's confusing, let's use dbg.value instead.

lib/Transforms/Utils/SimplifyCFG.cpp

2373

please clang-format.

test/CodeGen/X86/pr38762.ll

108

Perhaps compile with -gno-column-info, so the test input isn't quite as long?

[vsk]

include/llvm/Transforms/Utils/Local.h

454

I think function name and comments here could be clearer.

The action seems closer to 'hoisting' than 'promoting'. And the function is hoisting all of the instructions in the IfBlock, not just the phis. How about: 'hoistAllInstructionsInto'?

lib/Transforms/Utils/Local.cpp

2535

Right. Moving locations around degrades single-stepping and profiling. Moving dbg.values around makes variable updates appear re-ordered in a debug session.

[CarlosAlbertoEnciso]

include/llvm/Transforms/Utils/Local.h

454

Taking your suggested function name hoistAllInstructionsInto . Is a better description for what the function does. I have made some changes to the comments, hoping for more clarity.

lib/Transforms/Utils/Local.cpp

2535

Updated the comments to include a better explanation about the actions taken on DILocations and dbg.values.

2535

Taking your feedback, as the base for the updated comments.

2538

Included your description about the TODO work.

2540

this sentence is garbled :-)

Now reading my description, I realized that something was missing and it does not make much sense :-)

I have updated the comments and made a clear distinction between debug locations (DILocations) and debug.values.

lib/Transforms/Utils/SimplifyCFG.cpp

2373

Done.

test/CodeGen/X86/pr38762.ll

108

Used your suggested option -gno-column-info.

[CarlosAlbertoEnciso]

Address issues raised by the reviewers.

Improve the comments in order to add more clarity.
Reduce test case by using no-column-info option.
Better description and name for the new function.

Thanks for the comments and suggestions.

[aprantl]

The code looks fine, I have some comments about the comments.

lib/Transforms/Utils/Local.cpp

2535

Just to be clear: When I said "please explain" I meant: in the comment in the code, not to me in the review :-)

[aprantl]

LGTM with outstanding comments addressed. Thanks!

test/CodeGen/X86/pr38762.ll

52

Do we need all the !tbaa metadata for the test?

[CarlosAlbertoEnciso]

lib/Transforms/Utils/Local.cpp

2535

I have added extra comments in the code for the function 'hoistAllInstructionsInto'.

test/CodeGen/X86/pr38762.ll

52

Good point. I have removed all the !tbaa metadata. It is not required.

[CarlosAlbertoEnciso]

Address issues raised by the reviewers.

@vsk: Do you have any additional comments.

Thanks to the reviewers for their comments and suggestions.

[vsk]

The logic looks fine, but the comment in hoistAllInstructionsInto should be shorter. One or two sentences would be ideal. Readers can look at the test case for more context. Thanks!

[CarlosAlbertoEnciso]

Address issues raised by the reviewers.

Improve the comments in order to add more clarity.

Thanks for the comments and suggestions.

[vsk]

Thank you, lgtm.

[CarlosAlbertoEnciso]

Thanks very much to all the reviewers for your valuable comments and suggestions.

[mstorsjo]

This broke compilation for me, compiling some source files now fail due to triggered assertions, see PR39243 for details. I guess I'll revert this in a while unless there's someone to fix it soon.

[CarlosAlbertoEnciso]

In D52887#1260298, @mstorsjo wrote:

This broke compilation for me, compiling some source files now fail due to triggered assertions, see PR39243 for details. I guess I'll revert this in a while unless there's someone to fix it soon.

@mstorsjo, I am sorry about the issues caused by the commit. I am in the process of reverting the changes. Thanks.

[CarlosAlbertoEnciso]

@mstorsjo, The change has been reverted. Thanks for creating the bugzilla.

[CarlosAlbertoEnciso]

The patch caused some assertions. For more details see PR39243.

[CarlosAlbertoEnciso]

This revision caused builbot failures.

A new review have been created to address both issues D53287.

include/llvm/Transforms/Utils/Local.h

 /// This handles mapping it to nonnull metadata if the new load is a pointer
 /// load instead of an integer load and the range doesn't cover null.
 void copyRangeMetadata(const DataLayout &DL, const LoadInst &OldLI, MDNode *N,
                        LoadInst &NewLI);
 
 /// Remove the debug intrinsic instructions for the given instruction.
 void dropDebugUsers(Instruction &I);
 
+/// Promote the PHI node \p IfBlock to the dominant block \p DomBlock, by
+/// moving its instructions to the insertion point \p InsertPt.
+///
+/// The moved instructions receive the insertion point debug location and
+/// their debug intrinsic instructions are removed.
+void promotePHINodes(BasicBlock *DomBlock,Instruction *InsertPt,
+                     BasicBlock *IfBlock);
+
 //===----------------------------------------------------------------------===//
 //  Intrinsic pattern matching
 //
 
 /// Try to match a bswap or bitreverse idiom.
 ///
 /// If an idiom is matched, an intrinsic call is inserted before \c I. Any added
 /// instructions are returned in \c InsertedInsts. They will all have been added

lib/Transforms/Utils/Local.cpp

 
 void llvm::dropDebugUsers(Instruction &I) {
   SmallVector<DbgVariableIntrinsic *, 1> DbgUsers;
   findDbgUsers(DbgUsers, &I);
   for (auto *DII : DbgUsers)
     DII->eraseFromParent();
 }
 
+void llvm::promotePHINodes(BasicBlock *DomBlock,Instruction *InsertPt,
+                           BasicBlock *IfBlock) {
+  // Since we are moving the instructions out of its basic block, we do not
+  // retain their original debug location and debug intrinsic instructions.
+  // Doing so would degrade the debugging experience and adversely affect the
+  // accuracy of profiling information.
+  // There are some discussions (PR39141) about extending llvm.dbg.value to
+  // take more than one LLVM SSA value instead of dropping debug info.
+  // For now, when Use the insertion point debug location.
+  for (auto &I : *IfBlock) {
+    I.dropUnknownNonDebugMetadata();
+    dropDebugUsers(I);
+    I.setDebugLoc(InsertPt->getDebugLoc());
+  }
+  DomBlock->getInstList().splice(InsertPt->getIterator(),
+                                 IfBlock->getInstList(), IfBlock->begin(),
+                                 IfBlock->getTerminator()->getIterator());
+}
+
 namespace {
 
 /// A potential constituent of a bitreverse or bswap expression. See
 /// collectBitParts for a fuller explanation.
 struct BitPart {
   BitPart(Value *P, unsigned BW) : Provider(P) {
     Provenance.resize(BW);
   }

lib/Transforms/Utils/SimplifyCFG.cpp

static bool FoldTwoEntryPHINode(PHINode *PN, const TargetTransformInfo &TTI,
 
   // If we can still promote the PHI nodes after this gauntlet of tests,
   // do all of the PHI's now.
   Instruction *InsertPt = DomBlock->getTerminator();
   IRBuilder<NoFolder> Builder(InsertPt);
 
   // Move all 'aggressive' instructions, which are defined in the
   // conditional parts of the if's up to the dominating block.
-  if (IfBlock1) {
-    for (auto &I : *IfBlock1) {
-      I.dropUnknownNonDebugMetadata();
-      dropDebugUsers(I);
-    }
-    DomBlock->getInstList().splice(InsertPt->getIterator(),
-                                   IfBlock1->getInstList(), IfBlock1->begin(),
-                                   IfBlock1->getTerminator()->getIterator());
-  }
-  if (IfBlock2) {
-    for (auto &I : *IfBlock2) {
-      I.dropUnknownNonDebugMetadata();
-      dropDebugUsers(I);
-    }
-    DomBlock->getInstList().splice(InsertPt->getIterator(),
-                                   IfBlock2->getInstList(), IfBlock2->begin(),
-                                   IfBlock2->getTerminator()->getIterator());
-  }
+  if (IfBlock1)
+    promotePHINodes(DomBlock,InsertPt,IfBlock1);
+  if (IfBlock2)
+    promotePHINodes(DomBlock,InsertPt,IfBlock2);
 
   while (PHINode *PN = dyn_cast<PHINode>(BB->begin())) {
     // Change the PHI node into a select instruction.
     Value *TrueVal = PN->getIncomingValue(PN->getIncomingBlock(0) == IfFalse);
     Value *FalseVal = PN->getIncomingValue(PN->getIncomingBlock(0) == IfTrue);
 
     Value *Sel = Builder.CreateSelect(IfCond, TrueVal, FalseVal, "", InsertPt);
     PN->replaceAllUsesWith(Sel);

test/CodeGen/X86/pr38762.ll

+; RUN: opt < %s -S -simplifycfg | FileCheck %s
+
+; Note: This patch is a complement to pr38763.
+;
+; When SimplifyCFG changes the PHI node into a select instruction, the debug
+; information becomes ambiguous. It causes the debugger to display unreached
+; lines and invalid variable values.
+;
+; When in the debugger, on the line "if (read1 > 3)", and we step from the
+; 'if' condition, onto the addition, then back to the 'if' again, which is
+; misleading because that addition doesn't really "happen" (it's speculated).
+; Additionally, the result of the speculation is visible in the "brains"
+; variable when we step onto the "if" a second time, when it's technically
+; never calculated by the program.
+
+; IR generated with:
+; clang -S -g -O2 -emit-llvm pr38762.cpp -o pr38762.ll -mllvm -opt-bisect-limit=10
+
+; // pr38762.cpp
+; int main() {
+;   volatile int foo = 0;
+;   int read1 = foo;
+;   int brains = foo;
+; 
+;   if (read1 > 3) {
+;     brains *= 2;
+;     brains += 1;
+;   }
+; 
+;   return brains;
+; }
+
+; Change the debug locations associated with the PHI nodes being promoted, to
+; the debug locations from the insertion point in the dominant block.
+
+; CHECK-LABEL: entry
+; CHECK:  %cmp = icmp sgt i32 %foo.0., 3, !dbg !18
+; CHECK:  %mul = shl nsw i32 %foo.0.5, 1, !dbg !20
+; CHECK-NOT:  call void @llvm.dbg.value(metadata i32 %mul, metadata !15, metadata !DIExpression()), !dbg !25
+; CHECK:  %add = or i32 %mul, 1, !dbg !20
+; CHECK-NOT:  call void @llvm.dbg.value(metadata i32 %add, metadata !15, metadata !DIExpression()), !dbg !25
+; CHECK:  %brains.0 = select i1 %cmp, i32 %add, i32 %foo.0.5, !dbg !20
+
+; ModuleID = 'pr38762.cpp'
+source_filename = "pr38762.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+; Function Attrs: norecurse nounwind uwtable
+define dso_local i32 @main() local_unnamed_addr #0 !dbg !7 {
+entry:
+  %foo = alloca i32, align 4
+  %foo.0..sroa_cast = bitcast i32* %foo to i8*
+  store volatile i32 0, i32* %foo, align 4, !tbaa !18
+  %foo.0. = load volatile i32, i32* %foo, align 4, !tbaa !18
+  %foo.0.5 = load volatile i32, i32* %foo, align 4, !tbaa !18
+  call void @llvm.dbg.value(metadata i32 %foo.0.5, metadata !15, metadata !DIExpression()), !dbg !25
+  %cmp = icmp sgt i32 %foo.0., 3, !dbg !26
+  br i1 %cmp, label %if.then, label %if.end, !dbg !28
+
+if.then:                                          ; preds = %entry
+  %mul = shl nsw i32 %foo.0.5, 1, !dbg !29
+  call void @llvm.dbg.value(metadata i32 %mul, metadata !15, metadata !DIExpression()), !dbg !25
+  %add = or i32 %mul, 1, !dbg !31
+  call void @llvm.dbg.value(metadata i32 %add, metadata !15, metadata !DIExpression()), !dbg !25
+  br label %if.end, !dbg !32
+
+if.end:                                           ; preds = %if.then, %entry
+  %brains.0 = phi i32 [ %add, %if.then ], [ %foo.0.5, %entry ], !dbg !33
+  call void @llvm.dbg.value(metadata i32 %brains.0, metadata !15, metadata !DIExpression()), !dbg !25
+  ret i32 %brains.0, !dbg !35
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata) #2
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 8.0.0 (trunk 343753)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+!1 = !DIFile(filename: "pr38762.cpp", directory: ".")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 8.0.0 (trunk 343753)"}
+!7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 1, type: !8, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!15}
+!13 = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: !10)
+!15 = !DILocalVariable(name: "brains", scope: !7, file: !1, line: 4, type: !10)
+!18 = !{!19, !19, i64 0}
+!19 = !{!"int", !20, i64 0}
+!20 = !{!"omnipotent char", !21, i64 0}
+!21 = !{!"Simple C++ TBAA"}
+!25 = !DILocation(line: 4, column: 7, scope: !7)
+!26 = !DILocation(line: 6, column: 13, scope: !27)
+!27 = distinct !DILexicalBlock(scope: !7, file: !1, line: 6, column: 7)
+!28 = !DILocation(line: 6, column: 7, scope: !7)
+!29 = !DILocation(line: 7, column: 12, scope: !30)
+!30 = distinct !DILexicalBlock(scope: !27, file: !1, line: 6, column: 18)
+!31 = !DILocation(line: 8, column: 12, scope: !30)
+!32 = !DILocation(line: 9, column: 3, scope: !30)
+!33 = !DILocation(line: 0, scope: !7)
+!34 = !DILocation(line: 12, column: 1, scope: !7)
+!35 = !DILocation(line: 11, column: 3, scope: !7)

test/CodeGen/X86/pr38763.ll

 ; 
 ;   return result;
 ; }
 
 ; Remove the '@llvm.dbg.value' associated with 'result' for the true/false
 ; branches, as they becomes ambiguous.
 
 ; CHECK-LABEL: entry
-; CHECK:  %cmp = icmp eq i32 %foo.0., 4
-; CHECK:  %add = add nsw i32 %foo.0.4, 2, !dbg !18
+; CHECK:  %cmp = icmp eq i32 %foo.0., 4, !dbg !18
+; CHECK:  %add = add nsw i32 %foo.0.4, 2, !dbg !20
 ; CHECK-NOT: @llvm.dbg.value(metadata i32 %add
-; CHECK:  %sub = add nsw i32 %foo.0.4, -2, !dbg !21
+; CHECK:  %sub = add nsw i32 %foo.0.4, -2, !dbg !20
 ; CHECK-NOT: @llvm.dbg.value(metadata i32 %sub
 ; CHECK:  %result.0 = select i1 %cmp, i32 %add, i32 %sub
 ; CHECK:  call void @llvm.dbg.value(metadata i32 %result.0, metadata !12, metadata !DIExpression()), !dbg !17
 
 ; ModuleID = 'pr38763.cpp'
 source_filename = "pr38763.cpp"
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-pc-linux-gnu"
 
 ; Function Attrs: norecurse nounwind uwtable
 define dso_local i32 @main() local_unnamed_addr #0 !dbg !7 {
 entry:
   %foo = alloca i32, align 4
   %foo.0..sroa_cast = bitcast i32* %foo to i8*
   store volatile i32 4, i32* %foo, align 4, !tbaa !19
   %foo.0. = load volatile i32, i32* %foo, align 4
   %foo.0.4 = load volatile i32, i32* %foo, align 4
   call void @llvm.dbg.value(metadata i32 0, metadata !16, metadata !DIExpression()), !dbg !27
-  %cmp = icmp eq i32 %foo.0., 4
-  br i1 %cmp, label %if.then, label %if.else
+  %cmp = icmp eq i32 %foo.0., 4, !dbg !28
+  br i1 %cmp, label %if.then, label %if.else, !dbg !30
 
 if.then:                                          ; preds = %entry
   %add = add nsw i32 %foo.0.4, 2, !dbg !31
   call void @llvm.dbg.value(metadata i32 %add, metadata !16, metadata !DIExpression()), !dbg !27
   br label %if.end
 
 if.else:                                          ; preds = %entry
   %sub = add nsw i32 %foo.0.4, -2, !dbg !34

 !10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
 !11 = !{!16}
 !16 = !DILocalVariable(name: "result", scope: !7, file: !1, line: 6, type: !10)
 !19 = !{!20, !20, i64 0}
 !20 = !{!"int", !21, i64 0}
 !21 = !{!"omnipotent char", !22, i64 0}
 !22 = !{!"Simple C++ TBAA"}
 !27 = !DILocation(line: 6, column: 7, scope: !7)
+!28 = !DILocation(line: 7, column: 12, scope: !29)
 !29 = distinct !DILexicalBlock(scope: !7, file: !1, line: 7, column: 7)
+!30 = !DILocation(line: 7, column: 7, scope: !7)
 !31 = !DILocation(line: 8, column: 20, scope: !32)
 !32 = distinct !DILexicalBlock(scope: !29, file: !1, line: 7, column: 18)
 !34 = !DILocation(line: 10, column: 20, scope: !35)
 !35 = distinct !DILexicalBlock(scope: !29, file: !1, line: 9, column: 10)