[PATCH] D119232: [DebugInfo][InstrRef] Don't create duplicate instruction numbers in X86-fixup-LEAs

[Jeremy Morse via Phabricator via llvm-commits]

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

Over in D104684 <https://reviews.llvm.org/D104684> we ended up creating duplicate instruction numbers for instruction-referencing mode (root cause: I haven't landed the docs in D113586 <https://reviews.llvm.org/D113586>). A new pattern for optimising LEAs is added, and the instruction number for the computed value is attached to two instructions, not one. This then causes an assertion to fire in EXPENSIVE_CHECKS mode as reported by @uabelho in [0]. This patch removes the duplicate instruction number and adds a test.

Without EXPENSIVE_CHECKs, the net effect is that the developer will be presented with the wrong variable location, so this shouldn't be crashing anywhere else. However, that's a good argument for adding more instr-ref checks to MachineVerifier.

(This patch also modifies a COREI7-LABEL line that looks like it was typo'd when I originally landed the test, ooof. It doesn't affect test coverage until this patch though).

[0] https://github.com/llvm/llvm-project/commit/6e03a68b776dc06826dacbdab26d24a90bb2173b#commitcomment-66240657


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119232

Files:
  llvm/lib/Target/X86/X86FixupLEAs.cpp
  llvm/test/DebugInfo/MIR/InstrRef/x86-lea-fixup.mir


Index: llvm/test/DebugInfo/MIR/InstrRef/x86-lea-fixup.mir
===================================================================
--- llvm/test/DebugInfo/MIR/InstrRef/x86-lea-fixup.mir
+++ llvm/test/DebugInfo/MIR/InstrRef/x86-lea-fixup.mir
@@ -1,6 +1,6 @@
-# RUN: llc -run-pass x86-fixup-LEAs -mtriple=x86_64-gnu-unknown -verify-machineinstrs -mcpu=corei7-avx -o - %s | FileCheck %s --check-prefix=COREI7
-# RUN: llc -run-pass x86-fixup-LEAs -mtriple=x86_64-gnu-unknown -verify-machineinstrs -mcpu=haswell -o - %s | FileCheck %s --check-prefix=HASWELL
-# RUN: llc -run-pass x86-fixup-LEAs -mtriple=x86_64-unknown-unknown -verify-machineinstrs -mcpu=atom -o - %s | FileCheck %s --check-prefix=ATOM
+# RUN: llc -run-pass x86-fixup-LEAs -mtriple=x86_64-gnu-unknown -verify-machineinstrs -mcpu=corei7-avx -o - %s | FileCheck %s --check-prefixes=COREI7,CHECK
+# RUN: llc -run-pass x86-fixup-LEAs -mtriple=x86_64-gnu-unknown -verify-machineinstrs -mcpu=haswell -o - %s | FileCheck %s --check-prefixes=HASWELL,CHECK
+# RUN: llc -run-pass x86-fixup-LEAs -mtriple=x86_64-unknown-unknown -verify-machineinstrs -mcpu=atom -o - %s | FileCheck %s --check-prefixes=ATOM,CHECK
 #
 # Test several LEA <=> ADD transformations that the fixup-leas pass performs,
 # and check that any debug-instr-number attached to the original instruction
@@ -9,7 +9,7 @@
 # in this file is only tested by one prefix / CPU mode. Some i386 specific
 # behaviours are in the -2 flavour of this file.
 ---
-# COREI7-LABE: name: pr43758
+# COREI7-LABEL: name: pr43758
 name:            pr43758
 alignment:       16
 tracksRegLiveness: true
@@ -75,3 +75,43 @@
     RET64 $eax
 
 ...
+---
+# CHECK-LABEL: testfour
+# In this code sequence, an LEA is converted into a subtract that's combined
+# with another subtract. The instruction number on the computed value should
+# only be attached to the last subtract.
+name:            testfour
+alignment:       16
+tracksRegLiveness: true
+tracksDebugUserValues: true
+liveins:
+  - { reg: '$esi' }
+frameInfo:
+  maxAlignment:    1
+  maxCallFrameSize: 0
+machineFunctionInfo: {}
+# CHECK:      debugValueSubstitutions:
+# CHECK-NEXT:  - { srcinst: 1, srcop: 0, dstinst: 2, dstop: 0, subreg: 0 }
+# CHECK-NEXT: constants:
+body:             |
+  bb.0.entry:
+    liveins: $esi
+
+    $eax = MOV32rr $esi, implicit-def $rax
+
+    renamable $ecx = LEA64_32r renamable $rax, 1, renamable $rax, 0, $noreg
+    renamable $edx = MOV32ri 1
+    renamable $edx = SUB32rr killed renamable $edx, killed renamable $ecx, implicit-def dead $eflags, debug-instr-number 1
+
+    ; CHECK:      MOV32ri 1
+    ; CHECK-NOT:  debug-instr-number
+    ; CHECK-NEXT: SUB32rr
+    ; CHECK-NOT:  debug-instr-number
+    ; CHECK-NEXT: SUB32rr
+    ; CHECK-SAME: debug-instr-number 2
+
+    MOV32mr $noreg, 1, $noreg, 0, $noreg, killed renamable $edx
+    $eax = KILL renamable $eax, implicit killed $rax
+    RET64 $eax
+
+...
Index: llvm/lib/Target/X86/X86FixupLEAs.cpp
===================================================================
--- llvm/lib/Target/X86/X86FixupLEAs.cpp
+++ llvm/lib/Target/X86/X86FixupLEAs.cpp
@@ -546,7 +546,6 @@
   if (KilledIndex)
     KilledIndex->setIsKill(false);
 
-  MBB.getParent()->substituteDebugValuesForInst(*AluI, *NewMI1, 1);
   MBB.getParent()->substituteDebugValuesForInst(*AluI, *NewMI2, 1);
   MBB.erase(I);
   MBB.erase(AluI);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D119232.406764.patch
Type: text/x-patch
Size: 3353 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220208/63be1fec/attachment.bin>