Page MenuHomePhabricator

[AArch64][DebugInfo] Do not recompute CalleeSavedStackSize
ClosedPublic

Authored by sdesmalen on Aug 29 2019, 3:34 AM.

Details

Summary

This patch fixes a bug exposed by D65653 where a subsequent invocation
of determineCalleeSaves ends up with a different size for the callee
save area, leading to different frame-offsets in debug information.

In the invocation by PEI, determineCalleeSaves tries to determine
whether it needs to spill an extra callee-saved register to get an
emergency spill slot. To do this, it calls 'estimateStackSize' and
manually adds the size of the callee-saves to this. PEI then allocates
the spill objects for the callee saves and the remaining frame layout
is calculated accordingly.

A second invocation in LiveDebugValues causes estimateStackSize to return
the size of the stack frame including the callee-saves. Given that the
size of the callee-saves is added to this, these callee-saves are counted
twice, which leads determineCalleeSaves to believe the stack has
become big enough to require spilling an extra callee-save as emergency
spillslot. It then updates CalleeSavedStackSize with a larger value.

Since CalleeSavedStackSize is used in the calculation of the frame
offset in getFrameIndexReference, this leads to incorrect offsets for
variables/locals when this information is recalculated after PEI.

Diff Detail

Event Timeline

sdesmalen created this revision.Aug 29 2019, 3:34 AM
vsk added a subscriber: vsk.Aug 29 2019, 11:34 AM

Why are we calling determineCalleeSaves in LiveDebugValues, anyway? Can't it just call getCalleeSavedInfo()?

Making the determineCalleeSaves computation lazy, like this patch does, is really confusing.

Why are we calling determineCalleeSaves in LiveDebugValues, anyway? Can't it just call getCalleeSavedInfo()?
Making the determineCalleeSaves computation lazy, like this patch does, is really confusing.

This method seems to be called by several passes:

  • LiveDebugValues
  • PrologEpilogInserter
  • RegUsageInfoCollector
  • RegisterScavenging
  • ShrinkWrap

The problem with determineCalleeSaves is that it does much more than determining the callee-saves:

  • It also creates a new StackObject for the emergency spill slot.
  • It marks this slot as the scavenging slot in the RegisterScavenger.
  • It saves the size of the callee-save area (in AArch64MachineFunctionInfo).

And this behaviour to change state seems somewhat by design, because RegisterScavenger* is passed as argument and MachineFunction is not a const reference. After PEI, getCalleeSavedInfo can be used as you suggest, but one could argue that determineCalleeSaves is still a valid interface to use. Perhaps I could add an assert in determineCalleeSaves that suggests using getCalleeSavedInfo if MFI.isCalleeSavedInfoValid() is true, but I think that is even more confusing. Perhaps the better way is to not let determineCalleeSaves change state at all, but that would require changes to the interface, which is likely to also require changes to other targets. From that point of view, letting determineCalleeSaves use the cached information from getCalleeSavedInfo seemed sensible.

This method seems to be called by several passes:

PrologEpilogInserter is expected to do this; it needs both the information, and the side-effects which modify the stack layout. And the call in RegisterScavenging is actually a unit-test which is trying to fake the state computed by PEI. So really, there are three places which are using the API after PrologEpilogInserter.

None of the places that use the API after PEI want any side-effects. And none of them actually use the functionality to update the RegisterScavenger state. LiveDebugValues and RegUsageInfoCollector both appear to just be using determineCalleeSaves as a bad substitute for getCalleeSavedInfo.

one could argue that determineCalleeSaves is still a valid interface to use

It seems much simpler to just say it isn't a valid interface to use after PEI.

sdesmalen updated this revision to Diff 218375.Sep 2 2019, 8:34 AM
  • Introduced a new method getCalleeSaves that fills in a BitVector with the callee-saves if isCalleeSavedInfoValid() is true.
  • Replaced uses of determineCalleeSaves with getCalleeSaves in LiveDebugValues, RegUsageInfoCollector and RegisterScavenging.
bjope added a subscriber: bjope.Sep 2 2019, 3:07 PM

This seems much better.

Did you intentionally skip updating ShrinkWrap? Should be straightforward. (There's a bunch of places in that file that reference a RegScavenger, but nothing actually uses it.)

llvm/include/llvm/CodeGen/TargetFrameLowering.h
294

Maybe add a comment here that this shouldn't be called by anything outside of PEI.

llvm/lib/CodeGen/RegisterScavenging.cpp
802

ScavengerTest might actually want the real determineCalleeSaves? In this context, it's basically acting as a substitute for PEI.

sdesmalen marked an inline comment as done.Sep 3 2019, 3:30 PM

This seems much better.

Thanks!

Did you intentionally skip updating ShrinkWrap? Should be straightforward. (There's a bunch of places in that file that reference a RegScavenger, but nothing actually uses it.)

Yes, ShrinkWrap still needs to use the determineCalleeSaves interface because it is run before PEI when CalleeSavedInfo in MachineFrameInfo has not yet been populated (and thus MFI.isCalleeSavedInfoValid() will return false).

efriedma accepted this revision.Sep 3 2019, 6:08 PM

LGTM

I'm afraid there's a latent bug in the interaction between ShrinkWrap and PEI, but I guess the effect might be sort of hard to spot; if we allocate an unnecessary stack object before PEI, it would be hard to notice.

This revision is now accepted and ready to land.Sep 3 2019, 6:08 PM
sdesmalen updated this revision to Diff 219316.Sep 9 2019, 5:24 AM
sdesmalen marked 2 inline comments as done.

Just before committing this patch, I realised that I had not yet tested it for all targets (I did this on a different machine than I normally use, hence the change in config).

This led to a few more unit-test failures and thus some small changes to the patch to fix those:

  • Made getCalleeSaves return an empty BitVector instead of asserting that CalleeSavedInfo.isValid(). The assert broke several tests that ran the LiveDebugValues pass in isolation. The MIR Parser only calls setCalleeSavedInfoValid(true) when there is at least one callee-save. So without any callee-saves specified in the MIR file, the assert would otherwise still trigger. It may be worth fixing this in a separate patch by e.g. adding an extra field to the MIR.

Note that most of the tests did not require the callee-saved info for the thing they were testing. For two of them,test/DebugInfo/MIR/{Mips,X86}/live-debug-values-reg-copy.mir, it was fixed by letting it start-before=prologepilog to populate the callee-saved info.

  • Made getCalleeSaves a virtual function. D64986 introduced a change for the ARM target to mark R0 as callee saved in determineCalleeSaves if the parameter passed in R0 is returned as-is (determined from the returned parameter attribute). Given that R0 is not an actual callee-saved register, the register does not end up in the CalleeSaved list in MachineFrameInfo. By making getCalleeSaves a virtual function, the ARM target can still mark R0 as a preserved register.

@efriedma, are you happy with these changes?

This revision was automatically updated to reflect the committed changes.
omjavaid reopened this revision.Oct 7 2019, 9:31 PM

Hi

This change was reverted and hence causes LLDB AArch64 test failures again.

Revert action documented here : https://reviews.llvm.org/D67710

This revision is now accepted and ready to land.Oct 7 2019, 9:31 PM

Thanks for pointing that out.

Yes, thanks for pointing out! I've created a fix for the issue in D68783.

sdesmalen closed this revision.Nov 5 2019, 6:30 AM