[DebugInfo][InstrRef] Fix a broken substitution method, add test coverage

[jmorse]

Summary

This patch fixes a clearly-broken function that I absent-mindedly bodged many months ago.

Over in D85749 I landed the substituteDebugValuesForInst, that creates substitution records for all the def operands from one debug-labelled instruction to the new one. Unfortunately it would crash if the two instructions had different numbers of operands; I tried to fix this in 537f0fbe82 by adding a "max operand" parameter to the method, but then didn't actually change the loop bound to take account of this. It passed all the tests because.... well there wasn't any real test coverage of this method.

This patch fixes up the loop to be bounded by the MaxOperand bound; and adds test coverage for the x86-fixup-LEAs calls to this method, so that it's actually tested.

Ooops.

[Orlando]

The tests look good. LGTM!

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

4

nit: It loos like this file is just testing LEA => ADD (rather than LEA <=> ADD), or am I missing something?

41

What is this comment for?

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

6

I'm finding it difficult to understand this opening sentence and nit: labeles -> labels

[jmorse]

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

4

I guess the comment is misleading; between the two files, all instances of eraseFromParent (and the substitute calls I put in at those sites) are covered. They're in two different files because we need to cover both i386 and x86_64. I'll fiddle with the comments to reflect this.

41

Reminding myself what flags were required to stimulate the substitution -- hence this file targets i386, but tunes the cpu to corei7. I'll delete this when committing.

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

6

Rewriting when landing.

llvm/lib/CodeGen/MachineFunction.cpp

if (!OldInstrNum)
     return;
 
   // Iterate over all operands looking for defs to create substitutions for.
   // Avoid creating new instr numbers unless we create a new substitution.
   // While this has no functional effect, it risks confusing someone reading
   // MIR output.
   // Examine all the operands, or the first N specified by the caller.
   MaxOperand = std::min(MaxOperand, Old.getNumOperands());
-  for (unsigned int I = 0; I < Old.getNumOperands(); ++I) {
+  for (unsigned int I = 0; I < MaxOperand; ++I) {
     const auto &OldMO = Old.getOperand(I);
     auto &NewMO = New.getOperand(I);
     (void)NewMO;
 
     if (!OldMO.isReg() || !OldMO.isDef())
       continue;
     assert(NewMO.isDef());
 

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

+# RUN: llc -run-pass x86-fixup-LEAs -mtriple=i386 -verify-machineinstrs -mcpu=corei7-avx -o - %s | FileCheck %s
+#
+# Test that several LEA => ADD transforms get substitutions applied to them,
+# for corner cases that we can only hit with -mtriple=i386.
+---
+name:            test2add_32
+# CHECK: name: test2add_32
+alignment:       16
+tracksRegLiveness: true
+liveins:
+  - { reg: '$eax' }
+  - { reg: '$ebp' }
+# CHECK:       debugValueSubstitutions:
+# CHECK-NAME:  - { srcinst: 1, srcop: 0, dstinst: 2, dstop: 0, subreg: 0 }
+body:             |
+  bb.0:
+    liveins: $eax, $ebp
+
+    ; CHECK: $eax = ADD32ri8 {{.*}} debug-instr-number 2
+    $eax = LEA32r killed $eax, 1, killed $ebp, -5, $noreg, debug-instr-number 1
+    RETQ $eax
+
+...
+---
+name:            test1mov1add_ebp_32
+# CHECK-LABEL: name: test1mov1add_ebp_32
+alignment:       16
+tracksRegLiveness: true
+liveins:
+  - { reg: '$eax' }
+  - { reg: '$ebx' }
+  - { reg: '$ebp' }
+# CHECK:       debugValueSubstitutions:
+# CHECK-NEXT:  - { srcinst: 1, srcop: 0, dstinst: 2, dstop: 0, subreg: 0 }
+body:             |
+  bb.0:
+    liveins: $eax, $ebp, $ebx
+
+    ; CHECK: $ebx = ADD32rr {{.*}} debug-instr-number 2
+    $ebx = LEA32r killed $ebp, 1, $ebp, 0, $noreg, debug-instr-number 1
+    RETQ $ebx
+
+...
+---
+name:            testleaadd_ebp_index_32
+# CHECK-LABEL: name: testleaadd_ebp_index_32
+alignment:       16
+tracksRegLiveness: true
+liveins:
+  - { reg: '$ebx' }
+  - { reg: '$ebp' }
+# CHECK:       debugValueSubstitutions:
+# CHECK-NEXT:  - { srcinst: 1, srcop: 0, dstinst: 2, dstop: 0, subreg: 0 }
+body:             |
+  bb.0:
+    liveins: $eax, $ebp, $ebx
+
+    ; CHECK: $ebx = ADD32rr {{.*}} debug-instr-number 2
+    $ebx = LEA32r $ebp, 1, $ebp, 5, $noreg, debug-instr-number 1
+    RETQ $ebx
+
+...

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
+#
+# 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
+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 }
+body:             |
+  bb.0:
+    liveins: $rax, $rbp
+
+    $ebp = LEA64_32r killed $rbp, 1, killed $rax, 0, $noreg, debug-instr-number 1
+    ; COREI7: ADD32rr {{.*}} debug-instr-number 2
+    RETQ $ebp
+
+...
+---
+name:            test_mul_spec
+# HASWELL-LABEL: name: test_mul_spec
+alignment:       16
+tracksRegLiveness: true
+liveins:
+  - { reg: '$edi' }
+frameInfo:
+  maxAlignment:    1
+  maxCallFrameSize: 0
+machineFunctionInfo: {}
+# HASWELL:      debugValueSubstitutions:
+# HASWELL-NEXT:  - { srcinst: 1, srcop: 0, dstinst: 3, dstop: 0, subreg: 0 }
+# HASWELL-NEXT:  - { srcinst: 2, srcop: 0, dstinst: 4, dstop: 0, subreg: 0 }
+body:             |
+  bb.0:
+    liveins: $edi
+  
+    renamable $edi = KILL $edi, implicit-def $rdi
+    renamable $ecx = nsw LEA64_32r renamable $rdi, 8, renamable $rdi, 42, $noreg, debug-instr-number 1
+    ; HASWELL: ADD32ri8 {{.*}} debug-instr-number 3
+    renamable $eax = nsw LEA64_32r killed renamable $rdi, 4, renamable $rdi, 2, $noreg, debug-instr-number 2
+    ; HASWELL: ADD32ri8 {{.*}} debug-instr-number 4
+    renamable $eax = nsw IMUL32rr killed renamable $eax, killed renamable $ecx, implicit-def dead $eflags
+    RETQ $eax
+
+...
+---
+name:            testthree
+# ATOM-LABEL: name: testthree
+alignment:       16
+tracksRegLiveness: true
+frameInfo:
+  maxAlignment:    1
+  maxCallFrameSize: 0
+machineFunctionInfo: {}
+# ATOM:       debugValueSubstitutions:
+# ATOM-NEXT:  - { srcinst: 1, srcop: 0, dstinst: 2, dstop: 0, subreg: 0 }
+body:             |
+  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`)
+    RETQ $eax
+
+...