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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope 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 {
----------------
vsk wrote:
> 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.
Ok, looks like it wasn't so many tests failing after all then. So I agree that we should try to adjust the too rigid tests first.
I'll also examine FrameLowering implementations to see if FrameDestroy is used by different targets.

It would ofcourse be nice to skip the transition state completely, but then I think we somehow should give out-of-tree targets a heads up.


Repository:
  rL LLVM

https://reviews.llvm.org/D50638