Page MenuHomePhabricator

[InstrRef][AArch64][2/4] Recognise post-fe spills and restores
Needs ReviewPublic

Authored by jmorse on Jun 18 2021, 4:54 AM.

Details

Reviewers
t.p.northover
david-arm
dmgreen
Group Reviewers
debug-info
Summary

An important feature of instruction-referencing LiveDebugValues is that it can track variable values that are both in registers and on the stack at the same time. However, AArch64 doesn't benefit from this behaviour unless it is able to report when an instruction is a load or store from the stack. This patch implements two TargetInstrInfo hooks to collect this information.

These two functions are effectively copy-and-pastes of the pre-frame-elimination hooks, except that they recover the frame index of the stack slot accessed by examining the memory operand attached to the instruction. Instructions are recognised by being LDR* / STR* with constant offsets from the stack pointer.

The test added takes a Value, clobbers all registers with inlineasm to force the value onto the stack, then loads and returns it. LiveDebugValues should recognise this and:

  • At the DBG_INST_REF mark the value as being on the stack with DBG_VALUE
  • When the stack location is overwritten with the subsequent STRXui, LiveDebugValue should recognise this and place the variable in $x0, where it was loaded to.

Diff Detail

Event Timeline

jmorse created this revision.Jun 18 2021, 4:54 AM
jmorse requested review of this revision.Jun 18 2021, 4:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 18 2021, 4:54 AM
aprantl added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.h
66

What does FE stand for? Maybe explain this in the doxygen comment unless this is well known to everyone working on the AArch64 backend?

jmorse updated this revision to Diff 356738.Tue, Jul 6, 8:22 AM

Refresh this patch, as I'd uh, failed to notice the test changes that resulted from it. 100% of the changes are removing the word 'Folded" from the textual assembly comments, this happens simply because the assembly printer can now recognise some instructions as being proper spills, rather than folded spills. No actual output changes at all.

(Not sure why I missed this ._.)

Hi AArch64 developers (not sure who to add as reviewers), I'd like to implement the isStoreToStackSlotPostFE / isLoadFromStackSlotPostFE target hooks for AArch64, so that post-frame-elimination code can recognise spills and restores. This is important for debug-info, as we need help to track variable values as they move on and off the stack. The implementation is almost identical to the pre-frame-elimination target hooks, although instead of filtering for loads/stores that operate on frame-indexes, I've filtered for loads/stores that have stack memory operands and operate relative to the stack pointer.

There are no codegen changes in the tests -- all the test changes in this patch are the deletion of the word "Folded" from autogenerated tests, the assembly printer puts it in there if it doesn't recognise an instruction as a "proper" spill or restore.

In terms of potential codegen changes, nothing in llvm (other than debug-info) uses these hooks, except for the "getSpillSize" / "getRestoreSize" target hooks. There's a small chance that a machine-independent optimisation might behave differently as a result of this.

llvm/lib/Target/AArch64/AArch64InstrInfo.h
66

Here it's "frame elimination", i.e. after the stack slots are allocated spaces. You're right that this is probably of interest to the aarch64 maintainers, who I'll now add as reviewers...,

jmorse updated this revision to Diff 356765.Tue, Jul 6, 9:30 AM

Add back the debug-info feature test in recognise-spill-restore.mir, accidentally dropped it in last update :(

efriedma added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
2190

Why does it matter if the address computation SP, as opposed to the frame pointer or a scratch register? It's a load from a stack slot either way.

I happened to be looking at these functions for a different reason yesterday. Out of interest, is there anything that really makes them target dependent? It seems like it should be able to check what it needs via the MMO's PseudoValue without needing to look at opcodes (or regs, as Eli mentioned).

jmorse added a comment.Wed, Jul 7, 8:59 AM

I happened to be looking at these functions for a different reason yesterday. Out of interest, is there anything that really makes them target dependent? It seems like it should be able to check what it needs via the MMO's PseudoValue without needing to look at opcodes (or regs, as Eli mentioned).

I believe there's no target-independent way to identify the store-source register, which is what debug-info is interested in. On AArch64 it's the 3rd operand, on X86 it's the 6th (see sibling patch D105428). Previously debug-info has been looking for a killed register and assuming that that's the store-source, however there are various scenarios where spills don't kill registers.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
2190

I guess it doesn't -- I've been focusing on scenarios that involve spills and restores only, rather than general stack stores and loads. I'll revise this to not include the SP check.

I happened to be looking at these functions for a different reason yesterday. Out of interest, is there anything that really makes them target dependent? It seems like it should be able to check what it needs via the MMO's PseudoValue without needing to look at opcodes (or regs, as Eli mentioned).

I believe there's no target-independent way to identify the store-source register, which is what debug-info is interested in. On AArch64 it's the 3rd operand, on X86 it's the 6th (see sibling patch D105428). Previously debug-info has been looking for a killed register and assuming that that's the store-source, however there are various scenarios where spills don't kill registers.

Oh you are returning the register. None of the other implementations of isStoreToStackSlotPostFE seem to do that, they return 1/0 or true/false. It doesn't appear to be documented anywhere what it should be returning, other than the docs for isStoreToStackSlot. It might be worth shoring that up a bit. Either deciding that it returns a register and altering the existing implementations to match, or deciding it's a boolean (which might remove the need for a target method altogether, depending on whether checking the MMO is OK).