[DebugInfo][InstrRef] Fix over-droppage of locations from X86FloatingPoint fixup pass

[jmorse]

Summary

Over in D105657, we started dropping instruction numbers (that become variable locations) from call instructions, as we can't correctly represent the x87 FP stack. Unfortunately, it turns out that the "special FP instructions" that this pass transforms includes "every call instruction" [0]. Thus, we've ended up dropping all return values from all calls. Ouch.

This patch adds a filter: only drop instruction numbers from calls if they return something on the FP stack. Seeing how LLVM only allows a single return value, this should drop instruction numbers on anything that returns a float, and nothing else.

Rather than writing a new test, I've modified the original one to have a positive and negative case: drop instruction number on a call with an FP-stack modification, keep it on a plain call.

(This patch removes a check for "ADD_F64m" -- this was only put in to ensure that we didn't accept spurious inputs, like a stack trace).

[0] https://github.com/llvm/llvm-project/blob/f46321207f7d28f21d0dfb3635933d1bd84b5e05/llvm/lib/Target/X86/X86FloatingPoint.cpp#L433

[djtodoro]

Thanks!

[pengfei]

Some nits.

llvm/lib/Target/X86/X86FloatingPoint.cpp

1040–1041

Should update the comments?

llvm/test/DebugInfo/MIR/InstrRef/x86-fp-stackifier-drop-locations.mir

8

Should check the next CALLpcrel32 dropped debug-instr-number?

[jmorse]

llvm/lib/Target/X86/X86FloatingPoint.cpp

1040–1041

True -- I'll fold that in, thanks for the pointer.

llvm/test/DebugInfo/MIR/InstrRef/x86-fp-stackifier-drop-locations.mir

8

It should be caught by the --implicit-check-not on the FileCheck command line

llvm/lib/Target/X86/X86FloatingPoint.cpp

#endif
   // Reset the FP Stack - It is required because of possible leftovers from
   // passed arguments. The caller should assume that the FP stack is
   // returned empty (unless the callee returns values on FP stack).
   while (StackTop > 0)
     popReg();
 
   for (unsigned I = 0; I < N; ++I)
     pushReg(N - I - 1);
 
-  // Drop all variable values defined by this call -- we can't track them
-  // once they've been stackified.
+  // If this call has been modified, drop all variable values defined by it.
+  // We can't track them once they've been stackified.
+  if (STReturns)
     I->dropDebugNumber();
 }
 
 /// If RET has an FP register use operand, pass the first one in ST(0) and
 /// the second one in ST(1).
 void FPS::handleReturn(MachineBasicBlock::iterator &I) {
   MachineInstr &MI = *I;
 
   // Find the register operands.

llvm/test/DebugInfo/MIR/InstrRef/x86-fp-stackifier-drop-locations.mir

-# RUN: llc %s -run-pass=x86-codegen -o - -experimental-debug-variable-locations | FileCheck %s
+# RUN: llc %s -run-pass=x86-codegen -o - -experimental-debug-variable-locations | FileCheck %s --implicit-check-not=debug-instr-number
 #
 # The x87 FP instructions below have debug instr numbers attached -- but the
 # operands get rewritten when it's converted to stack-form. Rather than trying
 # to recover from this, drop any instruction numbers.
 #
-# CHECK-NOT: debug-instr-number
-# CHECK:     ADD_F64m
-# CHECK-NOT: debug-instr-number
+## We shouldn't drop numbers from _every_ call though:
+# CHECK:  CALLpcrel32 @ext, csr_32, implicit $esp, implicit $ssp, implicit-def $esp, implicit-def $ssp, debug-instr-number 101,
 #
 # Original program, command line 'clang ./test.c -O2 -g -m32 -o out.o -c'
 #
 # long double ext();
 # 
 # long double glob = 1.234;
 # 
 # long double foo(long double a, long double b, long double c) {
bb.0.entry:
     renamable $fp0 = nofpexcept LD_Fp80m %fixed-stack.0, 1, $noreg, 0, $noreg, implicit-def dead $fpsw, implicit $fpcw, debug-instr-number 1 :: (load (s80) from %fixed-stack.0, align 8)
     ST_FpP80m %stack.0, 1, $noreg, 0, $noreg, killed renamable $fp0, implicit-def $fpsw, implicit $fpcw, debug-instr-number 2 :: (store (s80) into %stack.0, align 4)
     renamable $fp1 = nofpexcept LD_Fp80m %fixed-stack.1, 1, $noreg, 0, $noreg, implicit-def dead $fpsw, implicit $fpcw, debug-instr-number 3 :: (load (s80) from %fixed-stack.1, align 4)
     ST_FpP80m %stack.1, 1, $noreg, 0, $noreg, renamable $fp1, implicit-def $fpsw, implicit $fpcw, debug-instr-number 4 :: (store (s80) into %stack.1, align 4)
     renamable $fp0 = nofpexcept LD_Fp80m %fixed-stack.2, 1, $noreg, 0, $noreg, implicit-def dead $fpsw, implicit $fpcw, debug-instr-number 5 :: (load (s80) from %fixed-stack.2, align 16)
     renamable $fp0 = nofpexcept ADD_Fp80 killed renamable $fp0, killed renamable $fp1, implicit-def dead $fpsw, implicit $fpcw, debug-instr-number 6, debug-location !21
     ST_FpP80m %stack.2, 1, $noreg, 0, $noreg, killed renamable $fp0, implicit-def $fpsw, implicit $fpcw, debug-instr-number 7 :: (store (s80) into %stack.2, align 4)
     ADJCALLSTACKDOWN32 0, 0, 0, implicit-def dead $esp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $esp, implicit $ssp, debug-location !22
+
+    ; This identical call (to the one below) does not touch the FP registers at
+    ; all. Therefore, it shouldn't have any debug instructions dropped.
+    CALLpcrel32 @ext, csr_32, implicit $esp, implicit $ssp, implicit-def $esp, implicit-def $ssp, debug-instr-number 101, debug-location !22
+
+    ; Original call: this should have it's location dropped, as it touches the
+    ; FP stack.
     CALLpcrel32 @ext, csr_32, implicit $esp, implicit $ssp, implicit-def $esp, implicit-def $ssp, implicit-def $fp0, debug-instr-number 100, debug-location !22
+
+
     ADJCALLSTACKUP32 0, 0, implicit-def dead $esp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $esp, implicit $ssp, debug-location !22
     renamable $fp1 = LD_Fp80m %stack.2, 1, $noreg, 0, $noreg, implicit-def $fpsw, implicit $fpcw :: (load (s80) from %stack.2, align 4)
     renamable $fp0 = nofpexcept MUL_Fp80 killed renamable $fp1, killed renamable $fp0, implicit-def dead $fpsw, implicit $fpcw, debug-instr-number 8, debug-location !23
     ST_FpP80m %stack.2, 1, $noreg, 0, $noreg, killed renamable $fp0, implicit-def $fpsw, implicit $fpcw, debug-instr-number 9 :: (store (s80) into %stack.2, align 4)
     ADJCALLSTACKDOWN32 0, 0, 0, implicit-def dead $esp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $esp, implicit $ssp, debug-location !24
     CALLpcrel32 @ext, csr_32, implicit $esp, implicit $ssp, implicit-def $esp, implicit-def $ssp, implicit-def $fp0, debug-location !24
     ADJCALLSTACKUP32 0, 0, implicit-def dead $esp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $esp, implicit $ssp, debug-location !24
     renamable $fp1 = nofpexcept LD_Fp32m80 $noreg, 1, $noreg, %const.0, $noreg, implicit-def dead $fpsw, implicit $fpcw, debug-instr-number 10 :: (load (s32) from constant-pool)