This is an archive of the discontinued LLVM Phabricator instance.

ARM: Compute MaxCallFrame size early
ClosedPublic

Authored by MatzeB on Apr 27 2017, 4:10 PM.

Details

Summary

This avoids
ARMBaseRegisterInfo::canRealignStack()/ARMFrameLowering::hasReservedCallFrame()
giving different answers in early/late phases of codegen.

The attached testcase shows a particular nasty example where we would call TargetRegisterInfo::needsStackRealignment()->canRealignStack()->hasReservedCallFrame() and would end up not aligning the stack because hasReservedCallFrame() would report false in late passes only.

This exposes a method in MachineFrameInfo that calculates
MaxCallFrameSize and calls it after instruction selection to get the value computed early.

To make this work in GlobalISel this also:

  • Move the TargetLoweringInfo::finalizeLowering code from IRTranslator to the end of InstructionSelect
  • Changing the MachineVerifier to work when the reserved registers are not frozen yet (by using TargetRegisterInfo::getReservedRegs() instead of MachineRegisterInfo stuff).

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB created this revision.Apr 27 2017, 4:10 PM
jmolloy added inline comments.
lib/Target/ARM/ARMBaseRegisterInfo.cpp
253

Is it right that if we don't have accurate call frame information we assume FP is not used here? Shouldn't we default to the conservative "true"?

MatzeB added inline comments.Apr 28 2017, 9:45 AM
lib/Target/ARM/ARMBaseRegisterInfo.cpp
253

Yes until the end of instruction selection we assume FP is not used here. This only affects ScheduleDAGRRList. It should only be used to direct the DAG scheduling heuristic and not affect correctness. Anyway I don't see this making a difference one way or the other as the machine scheduler will reorder anyway.

So I'm fine either way, let's switch to ? TFI->hasFP(MF) : true; then.

MatzeB added inline comments.Apr 28 2017, 9:47 AM
lib/Target/ARM/ARMBaseRegisterInfo.cpp
253

And to put this into context: The change in this function is needed when we want to enable the assert in https://reviews.llvm.org/D32570.

jmolloy accepted this revision.Apr 28 2017, 9:56 AM

OK, LGTM!

This revision is now accepted and ready to land.Apr 28 2017, 9:56 AM
MatzeB updated this revision to Diff 97317.May 1 2017, 11:23 AM
MatzeB added a reviewer: ab.

I had GlobalISel disabled in my earlier tests (cmake cache from times when it wasn't enabled yet).

I had to change GlobalISel to call TLI->finalizeLowering() late in InstructionSelect instead of after IRTranslator. While most ADJCALLSTACK/CALL instructions are already visible after IRTranslator we may miss operations getting implemented by library calls later. It also seems to delay freezing the reserved registers + extra adjustments until after ISel is finished. This also matches the current SelectionDAG behavior.

+Reviewers Ahmed because of the finalizeLowering/freezeReg movement.

I had to change GlobalISel to call TLI->finalizeLowering() late in InstructionSelect instead of after IRTranslator. While most ADJCALLSTACK/CALL instructions are already visible after IRTranslator we may miss operations getting implemented by library calls later. It also seems to delay freezing the reserved registers + extra adjustments until after ISel is finished. This also matches the current SelectionDAG behavior.

+Reviewers Ahmed because of the finalizeLowering/freezeReg movement.

And some more testing reveals that some GlobalISel tests use the machineverifier which in turn requires frozen reserved registers at the moment :-/. I will see how to change that...

MatzeB updated this revision to Diff 97363.May 1 2017, 4:42 PM
MatzeB edited the summary of this revision. (Show Details)
  • Change MachineVerifier to work if reserved registers are not frozen yet.
This revision was automatically updated to reflect the committed changes.