[llvm] [InstrRef] Preserve debug instr num in aarch64-ldst-opt. (PR #136009)

[Jeremy Morse via llvm-commits]

https://github.com/jmorse commented:

(Easter holidays and EuroLLVM kept me from this sorry),

I'm really glad instr-ref is able to address this really tricky transformation in aarch64; however I feel directly updating DBG_INSTR_REFs is the wrong direction here. Finding and updating the debug instructions involves scanning blocks and handling multiple DBG_INSTR_REFs (which this patch would need to do too). It'll be a pain on the (glorious but tragically future) day that we remove debug-instructions just like intrinsics.

Instead: I think the instruction/operand number substitution facility is useful here and will make it simpler too. The procedure would be give the new paired-load instruction a new instruction-number, then record the mapping between the old operands and the new ones. Mostly this would just be mapping {instr-num1, 0} and {instr-num2, 0} to {instr-num3, 0} and {instr-num3, 1}, i.e. the two load instructions operand zeros being mapped to operands zero and one of the new paired load instruction.

Once that's done and recorded in the debugSubstitutions field of the MIR output, when LiveDebugValues runs for each DBG_INSTR_REF it'll spot that there's a substitution, and forward *any* DBG_INSTR_REF referring to the old instr/operands to the new ones. Effectively this records some indirection (potentially even a chain of indirections) to avoid scanning for debug-instructions during optimisations: it all becomes a map lookup in LiveDebugValues.

Does this pass produce instructions with multiple memory operands, i.e. could two stores to the stack be merged into a paired-store with two different stack slots? LiveDebugValues tries to decode these to understand the movement of values from registers to the stack, and it might want extending to multiple operands, although not in this PR.

I think the test needs a CHECK-LABEL just to ensure we're testing the correct function each time. This also has a favourite test oddity of mine for the sign-extend case: the intention of the test is that the instruction-numbers assigned to the load and sign-extend are different, but FileCheck doesn't actually check that! The same instruction number can be captured into DBG_INSTR_NUM1 and DBG_INSTR_NUM2 and I believe that won't be a test failure. It's not a real issue; but it reminds me that we probably never added a machine-verifier that instruction-numbers never belong to more than one instruction. Probably because it was hard to solve at the time. (I guess I'll add that to the list of tech-debt...)

I see that the index of which operand gets sign-extended can be swapped based on `SExtIdx`. Seeing how that leads to some duplicate-ish code paths, the test coverage should cover each scenario handled. (Sorry to be the "more tests pls" guy).

https://github.com/llvm/llvm-project/pull/136009