[X86] Correctly return src/dest register from stack spill/restore recogniser hooks

[jmorse]

Summary

LLVM provides target hooks to recognise stack spill and restore instructions, such as isLoadFromStackSlot, and it also provides post frame elimination versions such as isLoadFromStackSlotPostFE. These are supposed to return the store-source and load-destination registers; unfortunately on X86, the PostFE recognisers just return "1", apparently to signify "yes it's a spill/load". This patch alters the hooks to correctly return the store-source and load-destination registers:

• The store source is the operand at position X86::AddrNumOperands -- so the first register after the memory operands. This is the same behaviour as pre frame elimination isStoreToStackSlot.
• The load destination register is at operand zero, in line with existing LLVM conventions.

This is really useful for debug-info (see child revision) as we it helps follow variable values as they move on/off the stack. There should be no codegen changes: the only other users of these PostFE target hooks are MachineInstr::getRestoreSize and MachineInstr::getSpillSize, which don't attempt to interpret the returned register location.

When these hooks were originally added [0] they came with a rider that they use a heuristic and so aren't reliable for correctness, which makes me cautious. However, I think this refers to the fact that the list of spill / restore instructions isn't necessarily complete, rather than the determination of the register location can't be guaranteed.

There's no test for this patch: because nothing codegen related actually uses this information. Instead, it's tested by a debug-info test, which I'll upload shortly and add as a child revision. (I figured it'd be better to not mix debug-info and X86 patches).

[0] https://github.com/jmorse/llvm-project/commit/2f4c37425b5ce7809601f0dbe5e45c9c0b17a17e

[jmorse]

Squashing the child commit into this so that the relevant test lands at the same time -- original review message for D105430 follows. The X86 backend part of this change allows us to identify the source register as r10 in the following:

MOV64mr $rsp, 1, $noreg, -8, $noreg, renamable $r10 :: (store 8 into %stack.0)

Which subsequently allows us to know that the value loaded into r10 earlier can be found in %stack.0.

~

Sometimes LLVM will generate code where values are both kept in a register temporarily, and spilt to the stack too. See the MIR in the attached test case, for example: r10 is loaded in the entry block, used in block three, and later restored in block eight. Variable location tracking couldn't deal with this because:

• We couldn't always determine the stored register from spilled instructions,
• VarLoc based LiveDebugValues could only track one location at a time for each variable.

This patches fix of the target hooks and InstrRef based LiveDebugValues fix these things respectively. We can now follow values in registers and on the stack at the same time. This means, for the attached test case, we can find a variable location on the stack at the position of the DBG_INSTR_REF, and emit corresponding DBG_VALUEs.

This patch deletes the old way of recognising store instruction registers, by looking for a kill flag on a register. Instead, rely fully on the target hook, isStoreToStackSlotPostFE, which returns the store-source register. This code was already in-tree, but hidden behind a flag, which this patch deletes.

Net X86 PC coverage gains with instruction referencing on a stage 2 clang build is 3%, which is... nice. (This forms part of what I've already reported on the mailing list).

[craig.topper]

This comment was removed by craig.topper.

[craig.topper]

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

2189 ↗

(On Diff #356516)

Doesn't ARMBaseInstrInfo::isStoreToStackSlotPostFE have the same issue?

[jmorse]

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

2189 ↗

(On Diff #356516)

It does indeed -- for the record, this new variable-location-tracking approach is hidden behind a flag (-experimental-debug-variable-locations), and I've been incrementally expanding the set of targets supported by it. X86 is almost entirely supported; AArch64 is progressing (see D104521 for example). ARM will likely come after, but I'd prefer to avoid fixing it until I can look at the target as a whole.

[craig.topper]

LGTM

llvm/lib/Target/X86/X86InstrInfo.cpp

if (isFrameLoadOpcode(MI.getOpcode(), Dummy)) {
     if ((Reg = isLoadFromStackSlot(MI, FrameIndex)))
       return Reg;
     // Check for post-frame index elimination operations
     SmallVector<const MachineMemOperand *, 1> Accesses;
     if (hasLoadFromStackSlot(MI, Accesses)) {
       FrameIndex =
           cast<FixedStackPseudoSourceValue>(Accesses.front()->getPseudoValue())
               ->getFrameIndex();
-      return 1;
+      return MI.getOperand(0).getReg();
     }
   }
   return 0;
 }
 
 unsigned X86InstrInfo::isStoreToStackSlot(const MachineInstr &MI,
                                           int &FrameIndex) const {
   unsigned Dummy;
if (isFrameStoreOpcode(MI.getOpcode(), Dummy)) {
     if ((Reg = isStoreToStackSlot(MI, FrameIndex)))
       return Reg;
     // Check for post-frame index elimination operations
     SmallVector<const MachineMemOperand *, 1> Accesses;
     if (hasStoreToStackSlot(MI, Accesses)) {
       FrameIndex =
           cast<FixedStackPseudoSourceValue>(Accesses.front()->getPseudoValue())
               ->getFrameIndex();
-      return 1;
+      return MI.getOperand(X86::AddrNumOperands).getReg();
     }
   }
   return 0;
 }
 
 /// Return true if register is PIC base; i.e.g defined by X86::MOVPC32r.
 static bool regIsPICBase(Register BaseReg, const MachineRegisterInfo &MRI) {
   // Don't waste compile time scanning use-def chains of physregs.