[framelowering] Improve tracking of first CS pop instruction.

[fhahn]

Summary

This patch makes sure FirstCSPop and MBBI never point to DBG_VALUE instructions, which affected the code generated.

[MatzeB]

The change itself looks good to me, but would be nice to simplify/cleanup the testcase.

lib/Target/X86/X86FrameLowering.cpp

1559–1565 ↗

(On Diff #80067)

You could use this to avoid code duplication:
`
if (Opc != DBG_VALUE && !Termiantor) {

if (Opc != Pop32 && Opc != Pop64)
  break;
FirstCSPop = PI;

}
`

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

4 ↗

(On Diff #80067)

The test seems bigger than necessary. Some things that could be simplified:

• Remove the ; preds = comments
• Simplify float constants to 0x0? Or use integer math in the first place?
• Do you need the calls to fmod, fabs?
• The nounwind, readnone attributes should not be necessary
• I don't know much about debuginfo, but if there is a way to reduce the amount of metadata at the end...

23 ↗

(On Diff #80067)

Use CHECK-LABEL noDebug: to improve FileCheck output in case of errors.

46 ↗

(On Diff #80067)

CHECK-LABEL

[fhahn]

Thanks for the review. I've updated the patch and hope I addressed all comments.

[fhahn]

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

4 ↗

(On Diff #80067)

I've reduced the test case quite a bit. AFAIK the debug data in the test case is the minimum required.

[MatzeB]

Thanks, LGTM. Nitpick below.

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

3 ↗

(On Diff #80330)

Nitpick: I prefer llc -o - %s over llc < %s because the former can be prefixed with lldb -- and still works as expected.

[fhahn]

Test run line nitpick.

[MatzeB]

The LGTM still stands. I often accept a review even with outstanding nitpicks when I trust that the author can fix the remaining items without the need for another review round.

llvm/trunk/lib/Target/X86/X86FrameLowering.cpp

if (RetOpcode && *RetOpcode == X86::CATCHRET) {
     BuildMI(MBB, MBBI, DL,
             TII.get(Is64Bit ? X86::POP64r : X86::POP32r), MachineFramePtr)
         .setMIFlag(MachineInstr::FrameDestroy);
   } else {
     NumBytes = StackSize - CSSize;
   }
   uint64_t SEHStackAllocAmt = NumBytes;
 
+  MachineBasicBlock::iterator FirstCSPop = MBBI;
   // Skip the callee-saved pop instructions.
   while (MBBI != MBB.begin()) {
     MachineBasicBlock::iterator PI = std::prev(MBBI);
     unsigned Opc = PI->getOpcode();
 
+    if (Opc != X86::DBG_VALUE && !PI->isTerminator()) {
       if ((Opc != X86::POP32r || !PI->getFlag(MachineInstr::FrameDestroy)) &&
-        (Opc != X86::POP64r || !PI->getFlag(MachineInstr::FrameDestroy)) &&
-        Opc != X86::DBG_VALUE && !PI->isTerminator())
+          (Opc != X86::POP64r || !PI->getFlag(MachineInstr::FrameDestroy)))
         break;
+      FirstCSPop = PI;
+    }
 
     --MBBI;
   }
-  MachineBasicBlock::iterator FirstCSPop = MBBI;
+  MBBI = FirstCSPop;
 
   if (TargetMBB) {
     // Fill EAX/RAX with the address of the target block.
     unsigned ReturnReg = STI.is64Bit() ? X86::RAX : X86::EAX;
     if (STI.is64Bit()) {
       // LEA64r TargetMBB(%rip), %rax
       BuildMI(MBB, FirstCSPop, DL, TII.get(X86::LEA64r), ReturnReg)
           .addReg(X86::RIP)

llvm/trunk/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) {
+  %call = call i64 @fn(i64 %a, i64 0)
+  ret i64 %call
+}
+
+; CHECK-LABEL: noDebug
+; CHECK: popq %rcx
+; CHECK: ret
+
+
+define i64 @withDebug(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: popq %rcx
+; CHECK: ret
+
+
+declare i64 @fn(i64, i64)
+
+declare void @llvm.dbg.value(metadata, i64, 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)