[framelowering] Skip dbg values when getting next/previous instruction.

[fhahn]

Summary

In mergeSPUpdates, debug values need to be ignored when getting the
previous element, otherwise debug data could have an impact on codegen.

In eliminateCallFramePseudoInstr, debug values after the erased element
could have an impact on codegen and should be skipped.

Closes PR31319 (https://llvm.org/bugs/show_bug.cgi?id=31319)

[aprantl]

In general this LGTM, but I wonder if we don't have / shouldn't introduce a more idiomatic way of skipping over DBG_VALUEs.

lib/Target/X86/X86FrameLowering.cpp

376

Would it make sense to introduce a better API for this? Skipping over all DBG_VALUEs is a pretty common operation.

test/CodeGen/X86/frame-lowering-debug-intrinsic-2.ll

21

should these be CHECK-NEXTs ?

[fhahn]

lib/Target/X86/X86FrameLowering.cpp

376

I think a better API for this would make sense. I'll have a look tomorrow and see there's an existing API for that or if a new API could be introduced.

[fhahn]

Updated patch to use skipDebugInstructions* functions introduced in D27782

[fhahn]

Use a few more CHECK-NEXT where possible.

@aprantl It would be great if you could have another look now that this patch is using generic functions to skip debug instructions.

[aprantl]

I think this looks good now. Thanks! (one comment inline)

lib/Target/X86/X86FrameLowering.cpp

383

Perhaps this would be easier to read if skipDebugInstructions* are on a subsequent line outside the :? expression?

[fhahn]

Moved skipDebugInstructionsBack/Forward to separate lines.

[fhahn]

@aprantl thank you very much for taking another look!

[fhahn]

Unfortunately this patch caused the sanitizer-x86_64-linux-autoconf builder to fail ( http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/2829 )

I've reverted the patch and will look into why it caused the failure in the next days.

[fhahn]

Updated eliminateCallFramePseudoInstr to return the next instruction even if it's a debug value, because the caller is responsible to replace frame index constants in debug values.

This fixes the buildbot regression.

[fhahn]

Please let me know if I should create a new review for the patch, because this review is already closed and I don't know how I could reopen it.

[mkuper]

Reopened. :-)

[fhahn]

thank you very much :)

The new diff changed quite a bit so I would appreciate if somebody could take another look.

[MatzeB]

LGTM with nitpicks.

lib/Target/X86/X86FrameLowering.cpp

2587–2588

This feels out of place in front of the function. (We should always have /// doxygen in front of the function, as this just describes just a small detail I would rather it next to the return. Though I find the call of whether to comment or not already close as the information is already given in the docu comment of TargetFrameLowering::eliminateCallFramePseudoInstr().

2597

How about InsertPos as a name?

[fhahn]

Thank you very much for your comments @MatzeB!

lib/Target/X86/X86FrameLowering.cpp

 int X86FrameLowering::mergeSPUpdates(MachineBasicBlock &MBB,
                                      MachineBasicBlock::iterator &MBBI,
                                      bool doMergeWithPrevious) const {
   if ((doMergeWithPrevious && MBBI == MBB.begin()) ||
       (!doMergeWithPrevious && MBBI == MBB.end()))
     return 0;
 
   MachineBasicBlock::iterator PI = doMergeWithPrevious ? std::prev(MBBI) : MBBI;
+  // Skip debug values.
+  while (PI != MBB.begin() && PI->isDebugValue())
+    --PI;
+
   MachineBasicBlock::iterator NI = doMergeWithPrevious ? nullptr
                                                        : std::next(MBBI);
+  // Skip debug values.
+  while (NI != nullptr && NI != MBB.end() && NI->isDebugValue())
+    ++NI;
+
   unsigned Opc = PI->getOpcode();
   int Offset = 0;
 
   if (!doMergeWithPrevious && NI != MBB.end() &&
       NI->getOpcode() == TargetOpcode::CFI_INSTRUCTION) {
     // Don't merge with the next instruction if it has CFI.
     return Offset;
   }
bool X86FrameLowering::adjustStackWithPops(MachineBasicBlock &MBB,
 
   for (int i = 0; i < NumPops; ++i)
     BuildMI(MBB, MBBI, DL, 
             TII.get(STI.is64Bit() ? X86::POP64r : X86::POP32r), Regs[i]);
 
   return true;
 }
 
 MachineBasicBlock::iterator X86FrameLowering::
 eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
                               MachineBasicBlock::iterator I) const {
   bool reserveCallFrame = hasReservedCallFrame(MF);
   unsigned Opcode = I->getOpcode();
   bool isDestroy = Opcode == TII.getCallFrameDestroyOpcode();
   DebugLoc DL = I->getDebugLoc();
   uint64_t Amount = !reserveCallFrame ? I->getOperand(0).getImm() : 0;
   uint64_t InternalAmt = (isDestroy || Amount) ? I->getOperand(1).getImm() : 0;
   I = MBB.erase(I);
 
+  // Skip debug values.
+  while (I != MBB.end() && I->isDebugValue())
+    ++I;
+
   if (!reserveCallFrame) {
     // If the stack pointer can be changed after prologue, turn the
     // adjcallstackup instruction into a 'sub ESP, <amt>' and the
     // adjcallstackdown instruction into 'add ESP, <amt>'
 
     // We need to keep the stack aligned properly.  To do this, we round the
     // amount of space needed for the outgoing arguments up to the next
     // alignment boundary.

test/CodeGen/X86/frame-lowering-debug-intrinsic-2.ll

+; Test ensuring debug intrinsics do not affect generated function prologue.
+;
+; RUN: llc -O1 -mtriple=x86_64-unknown-unknown -o - %s | FileCheck %s
+
+@a = local_unnamed_addr global i64 0, align 8
+
+define void @noDebug() {
+entry:
+  %0 = load i64, i64* @a, align 8
+  %1 = load i64, i64* @a, align 8
+  %2 = load i64, i64* @a, align 8
+  %3 = tail call { i64, i1 } @llvm.uadd.with.overflow.i64(i64 %0, i64 %1)
+  %4 = extractvalue { i64, i1 } %3, 0
+  %5 = tail call i64 @fn1(i64 %4, i64 %2)
+  tail call void (...) @printf()
+  tail call void (...) @printf(i64 1, i64 2, i64 3, i64 4, i32 0, i64 0, i64 %4, i64 %5)
+  ret void
+}
+
+; CHECK-LABEL: noDebug
+; CHECK: addq  $24, %rsp
+; CHECK: popq  %rbx
+; CHECK: popq  %r14
+; CHECK: retq
+
+
+define void @withDebug() !dbg !18 {
+entry:
+  %0 = load i64, i64* @a, align 8
+  %1 = load i64, i64* @a, align 8
+  %2 = load i64, i64* @a, align 8
+  %3 = tail call { i64, i1 } @llvm.uadd.with.overflow.i64(i64 %0, i64 %1)
+  %4 = extractvalue { i64, i1 } %3, 0
+  %5 = tail call i64 @fn1(i64 %4, i64 %2)
+  tail call void @llvm.dbg.value(metadata i64 %4, i64 0, metadata !23, metadata !33), !dbg !34
+  tail call void @llvm.dbg.value(metadata i64 %5, i64 0, metadata !22, metadata !33), !dbg !35
+  tail call void (...) @printf()
+  tail call void (...) @printf(i64 1, i64 2, i64 3, i64 4, i32 0, i64 0, i64 %4, i64 %5)
+  ret void
+}
+
+; CHECK-LABEL: withDebug
+; CHECK: #DEBUG_VALUE: test:j <- %RBX
+; CHECK: addq  $24, %rsp
+; CHECK: popq  %rbx
+; CHECK: popq  %r14
+; CHECK: retq
+
+declare { i64, i1 } @llvm.uadd.with.overflow.i64(i64, i64)
+declare i64 @fn1(i64, i64)
+
+declare void @printf(...)
+
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
+
+
+!llvm.dbg.cu = !{!1}
+!llvm.module.flags = !{!15, !16}
+
+!1 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !2, producer: "clang version 4.0.0")
+!2 = !DIFile(filename: "test.cpp", directory: "")
+!11 = !DIBasicType(name: "long int", size: 64, encoding: DW_ATE_signed)
+!15 = !{i32 2, !"Dwarf Version", i32 4}
+!16 = !{i32 2, !"Debug Info Version", i32 3}
+!18 = distinct !DISubprogram(name: "test", scope: !2, file: !2, line: 5, unit: !1)
+!22 = !DILocalVariable(name: "i", scope: !18, file: !2, line: 6, type: !11)
+!23 = !DILocalVariable(name: "j", scope: !18, file: !2, line: 7, type: !11)
+!33 = !DIExpression()
+!34 = !DILocation(line: 7, column: 17, scope: !18)
+!35 = !DILocation(line: 6, column: 8, scope: !18)
+!36 = !DILocation(line: 9, column: 3, scope: !18)
+!37 = !DILocation(line: 10, column: 10, scope: !18)

test/CodeGen/X86/frame-lowering-debug-intrinsic.ll

 ; Test ensuring debug intrinsics do not affect generated function prologue.
 ;
 ; RUN: llc -O1 -mtriple=x86_64-unknown-unknown -o - %s | FileCheck %s
 
-
-define i64 @noDebug(i64 %a) {
+define i64 @fn1NoDebug(i64 %a) {
   %call = call i64 @fn(i64 %a, i64 0)
   ret i64 %call
 }
 
-; CHECK-LABEL: noDebug
+; CHECK-LABEL: fn1NoDebug
 ; CHECK: popq %rcx
-; CHECK: ret
-
+; CHECK-NEXT: ret
 
-define i64 @withDebug(i64 %a) !dbg !4 {
+define i64 @fn1WithDebug(i64 %a) !dbg !4 {
   %call = call i64 @fn(i64 %a, i64 0)
   tail call void @llvm.dbg.value(metadata i64 %call, i64 0, metadata !5, metadata !6), !dbg !7
   ret i64 %call
 }
 
-; CHECK-LABEL: withDebug
+; CHECK-LABEL: fn1WithDebug
 ; CHECK: popq %rcx
-; CHECK: ret
+; CHECK-NEXT: ret
+
+%struct.Buffer = type { i8, [63 x i8] }
+
+define void @fn2NoDebug(%struct.Buffer* byval align 64 %p1) {
+  ret void
+}
+
+; CHECK-LABEL: fn2NoDebug
+; CHECK: and
+; CHECK-NOT: add
+; CHECK-NOT: sub
+; CHECK: mov
+; CHECK-NEXT: pop
+; CHECK-NEXT: ret
+
+define void @fn2WithDebug(%struct.Buffer* byval align 64 %p1) !dbg !4 {
+  call void @llvm.dbg.declare(metadata %struct.Buffer* %p1, metadata !5, metadata !6), !dbg !7
+  ret void
+}
 
+; CHECK-LABEL: fn2WithDebug
+; CHECK: and
+; CHECK-NOT: add
+; CHECK-NOT: sub
+; CHECK: mov
+; CHECK-NEXT: pop
+; CHECK-NEXT: ret
 
 declare i64 @fn(i64, i64)
 
 declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!2,!3}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 4.0.0")
 !1 = !DIFile(filename: "test.c", directory: "/")
 !2 = !{i32 2, !"Dwarf Version", i32 4}
 !3 = !{i32 2, !"Debug Info Version", i32 3}
 !4 = distinct !DISubprogram(name: "withDebug", unit: !0)
 !5 = !DILocalVariable(name: "w", scope: !4)
 !6 = !DIExpression()
 !7 = !DILocation(line: 210, column: 12, scope: !4)