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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse created this revision.
jmorse added reviewers: Orlando, StephenTozer, TWeaver, djtodoro.
Herald added subscribers: pengfei, hiraditya.
jmorse requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Over in D105657 <https://reviews.llvm.org/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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108580

Files:
  llvm/lib/Target/X86/X86FloatingPoint.cpp
  llvm/test/DebugInfo/MIR/InstrRef/x86-fp-stackifier-drop-locations.mir


Index: llvm/test/DebugInfo/MIR/InstrRef/x86-fp-stackifier-drop-locations.mir
===================================================================
--- llvm/test/DebugInfo/MIR/InstrRef/x86-fp-stackifier-drop-locations.mir
+++ llvm/test/DebugInfo/MIR/InstrRef/x86-fp-stackifier-drop-locations.mir
@@ -1,12 +1,11 @@
-# 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'
 #
@@ -147,7 +146,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
Index: llvm/lib/Target/X86/X86FloatingPoint.cpp
===================================================================
--- llvm/lib/Target/X86/X86FloatingPoint.cpp
+++ llvm/lib/Target/X86/X86FloatingPoint.cpp
@@ -1040,7 +1040,8 @@
 
   // Drop all variable values defined by this call -- we can't track them
   // once they've been stackified.
-  I->dropDebugNumber();
+  if (STReturns)
+    I->dropDebugNumber();
 }
 
 /// If RET has an FP register use operand, pass the first one in ST(0) and


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D108580.368194.patch
Type: text/x-patch
Size: 3145 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210823/9c9a8cd7/attachment.bin>