[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

22

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

379

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

2583–2584

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().

2593

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,
                                      bool doMergeWithPrevious) const {
   if ((doMergeWithPrevious && MBBI == MBB.begin()) ||
       (!doMergeWithPrevious && MBBI == MBB.end()))
     return 0;
 
   MachineBasicBlock::iterator PI = doMergeWithPrevious ? std::prev(MBBI) : MBBI;
   MachineBasicBlock::iterator NI = doMergeWithPrevious ? nullptr
                                                        : std::next(MBBI);
+  PI = skipDebugInstructionsBackward(PI, MBB.begin());
+  if (NI != nullptr)
+    NI = skipDebugInstructionsForward(NI, MBB.end());
+
   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);
+  auto InsertPos = skipDebugInstructionsForward(I, MBB.end());
 
   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
if (!reserveCallFrame) {
     // Amount == 0, because the preceding function may have set a non-0
     // GNU_ARGS_SIZE.
     // TODO: We don't need to reset this between subsequent functions,
     // if it didn't change.
     bool HasDwarfEHHandlers = !WindowsCFI && !MF.getLandingPads().empty();
 
     if (HasDwarfEHHandlers && !isDestroy &&
         MF.getInfo<X86MachineFunctionInfo>()->getHasPushSequences())
-      BuildCFI(MBB, I, DL,
+      BuildCFI(MBB, InsertPos, DL,
                MCCFIInstruction::createGnuArgsSize(nullptr, Amount));
 
     if (Amount == 0)
       return I;
 
     // Factor out the amount that gets handled inside the sequence
     // (Pushes of argument for frame setup, callee pops for frame destroy)
     Amount -= InternalAmt;
 
     // TODO: This is needed only if we require precise CFA.
     // If this is a callee-pop calling convention, emit a CFA adjust for
     // the amount the callee popped.
     if (isDestroy && InternalAmt && DwarfCFI && !hasFP(MF))
-      BuildCFI(MBB, I, DL, 
+      BuildCFI(MBB, InsertPos, DL,
                MCCFIInstruction::createAdjustCfaOffset(nullptr, -InternalAmt));
 
     // Add Amount to SP to destroy a frame, or subtract to setup.
     int64_t StackAdjustment = isDestroy ? Amount : -Amount;
     int64_t CfaAdjustment = -StackAdjustment;
 
     if (StackAdjustment) {
       // Merge with any previous or following adjustment instruction. Note: the
       // instructions merged with here do not have CFI, so their stack
       // adjustments do not feed into CfaAdjustment.
-      StackAdjustment += mergeSPUpdates(MBB, I, true);
-      StackAdjustment += mergeSPUpdates(MBB, I, false);
+      StackAdjustment += mergeSPUpdates(MBB, InsertPos, true);
+      StackAdjustment += mergeSPUpdates(MBB, InsertPos, false);
 
       if (StackAdjustment) {
         if (!(Fn->optForMinSize() &&
-              adjustStackWithPops(MBB, I, DL, StackAdjustment)))
-          BuildStackAdjustment(MBB, I, DL, StackAdjustment,
+              adjustStackWithPops(MBB, InsertPos, DL, StackAdjustment)))
+          BuildStackAdjustment(MBB, InsertPos, DL, StackAdjustment,
                                /*InEpilogue=*/false);
       }
     }
 
     if (DwarfCFI && !hasFP(MF)) {
       // If we don't have FP, but need to generate unwind information,
       // we need to set the correct CFA offset after the stack adjustment.
       // How much we adjust the CFA offset depends on whether we're emitting
       // CFI only for EH purposes or for debugging. EH only requires the CFA
       // offset to be correct at each call site, while for debugging we want
       // it to be more precise.
 
       // TODO: When not using precise CFA, we also need to adjust for the
       // InternalAmt here.
       if (CfaAdjustment) {
-        BuildCFI(MBB, I, DL, MCCFIInstruction::createAdjustCfaOffset(
-                                 nullptr, CfaAdjustment));
+        BuildCFI(MBB, InsertPos, DL,
+                 MCCFIInstruction::createAdjustCfaOffset(nullptr,
+                                                         CfaAdjustment));
       }
     }
 
     return I;
   }
 
   if (isDestroy && InternalAmt) {
     // If we are performing frame pointer elimination and if the callee pops

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-NEXT:  popq  %r14
+; CHECK-NEXT:  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-NEXT:  addq  $24, %rsp
+; CHECK:       popq  %rbx
+; CHECK-NEXT:  popq  %r14
+; CHECK-NEXT:  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)

test/DebugInfo/X86/dbg-value-frame-index.ll

+; RUN: llc -mtriple=x86_64-unknown-unknown -o - %s | FileCheck %s
+
+define i1 @test() !dbg !4 {
+entry:
+  %end = alloca i64, align 8
+  br label %while.cond
+
+while.cond:
+  call void @llvm.dbg.value(metadata i64* %end, i64 0, metadata !5, metadata !6), !dbg !7
+  %call = call i1 @fn(i64* %end, i64* %end, i64* null, i8* null, i64 0, i64* null, i32* null, i8* null), !dbg !7
+  br label %while.body
+
+while.body:
+  br i1 0, label %while.end, label %while.cond
+
+while.end:
+  ret i1 true
+}
+
+; CHECK-LABEL: test
+; CHECK:       #DEBUG_VALUE: test:w <- [%RSP+8]
+
+declare i1 @fn(i64*, i64*, i64*, i8*, i64, i64*, i32*, i8*)
+declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2,!3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 4.0.0", emissionKind: FullDebug)
+!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: "test", type: !10, unit: !0)
+!5 = !DILocalVariable(name: "w", scope: !4, type: !9)
+!6 = !DIExpression(DW_OP_deref)
+!7 = !DILocation(line: 210, column: 12, scope: !4)
+!8 = !{!9}
+!9 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean)
+!10 = !DISubroutineType(types: !8)