[PATCH] D50638: [AsmPrinter] Look inside bundles and rely on FrameDestroy in calculateDbgValueHistory

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: include/llvm/CodeGen/TargetFrameLowering.h:286
+  /// Check if the target is updating FrameSetup and FrameDestroy attributes on
+  /// machine instructions.
+  virtual bool isFrameSetupAndFrameDestroySupported() const {
----------------
bjope wrote:
> vsk wrote:
> > Do you know which backends don't mark up epilog instructions with FrameDestroy? Is it feasible to fix the backends? Seems like `collectChangingRegsInBody` is just a more precise way of doing the job of `collectChangingRegs`: it'd be great to just have one copy of the logic.
> Yes, the goal would be to make the hook return true in the future.
> 
> All I know right know is that turning this on by default makes lots of lit-tests fail. So even if the backend is updating FrameDestroy properly, we need to examine failing lit-tests, check if the target is using FrameDestroy, and update CHECK-statements when the result is improved.
> 
> A suggested deployment could be:
> 1) Add this hook (this patch).
> 2) For each in-tree target, examine if target is using FrameDestroy, if so override the hook and check/update lit-test. If not, start using FrameDestroy and override on hook.
> 3) When all in-tree targets are using collectChangingRegsInBody, we can remove the old collectChangingRegs (and getFirstEpilogueInst) functions.
> 
> A possible addition to step 3 is to add an assert that isFrameSetupAndFrameDestroySupported returns true. That would give out-of-tree targets a chance to detect that they need to adapt their code somehow. And then after awhile we can remove the hook completely.
With this patch applied I just see four failing tests in check-llvm:

    LLVM :: DebugInfo/AArch64/struct_by_value.ll
    LLVM :: DebugInfo/X86/parameters.ll
    LLVM :: DebugInfo/X86/reference-argument.ll
    LLVM :: DebugInfo/X86/vla-multi.ll

It seems like the failures result from the tests being too rigid, not from really fundamental issues. It seems like it'd be less work overall to fix these now (& I'd volunteer to help with that, just fixed the vla-multi.ll test in r340068). If there are more failing tests I'm just not seeing it'd make sense to reconsider, but I'd like to avoid leaving things in a transitionary state if possible.


Repository:
  rL LLVM

https://reviews.llvm.org/D50638