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

[jmorse]

Summary

Over in D104684 we ended up creating duplicate instruction numbers for instruction-referencing mode (root cause: I haven't landed the docs in 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

[StephenTozer]

Simple fix LGTM.

llvm/test/DebugInfo/MIR/InstrRef/x86-lea-fixup.mir

12

Nice drive-by fix.

llvm/lib/Target/X86/X86FixupLEAs.cpp

bool FixupLEAPass::optLEAALU(MachineBasicBlock::iterator &I,
   NewMI2->addRegisterDead(X86::EFLAGS, TRI);
 
   // Clear the old Kill flags.
   if (KilledBase)
     KilledBase->setIsKill(false);
   if (KilledIndex)
     KilledIndex->setIsKill(false);
 
-  MBB.getParent()->substituteDebugValuesForInst(*AluI, *NewMI1, 1);
   MBB.getParent()->substituteDebugValuesForInst(*AluI, *NewMI2, 1);
   MBB.erase(I);
   MBB.erase(AluI);
   I = NewMI1;
   return true;
 }
 
 bool FixupLEAPass::optTwoAddrLEA(MachineBasicBlock::iterator &I,

llvm/test/DebugInfo/MIR/InstrRef/x86-lea-fixup.mir

-# 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
 # is substituted onto the new instruction.
 # Some are only reachable under specific CPU modes it seems -- each function
 # 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
 liveins:
   - { reg: '$rax' }
   - { reg: '$rbp' }
 # COREI7:      debugValueSubstitutions:
 # COREI7-NEXT: - { srcinst: 1, srcop: 0, dstinst: 2, dstop: 0, subreg: 0 }
bb.0.entry:
     renamable $ecx = MOV32ri 0
     renamable $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags
     renamable $ecx = nsw ADD32rr renamable $ecx, renamable $eax, implicit-def dead $eflags, implicit killed $rax, implicit killed $rcx, implicit-def $rcx, debug-instr-number 1
     ; ATOM: LEA64_32r {{.*}} debug-instr-number 2
     renamable $eax = MOV32rm killed renamable $rcx, 1, $noreg, 0, $noreg :: (load (s32) from `i32 *undef`)
     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
+
+...