[llvm] r179500 - Fix PPC64 CR spill location for callee-saved registers
Bill Schmidt
wschmidt at linux.vnet.ibm.com
Mon Apr 15 18:59:02 PDT 2013
On Mon, 2013-04-15 at 18:32 -0500, Hal Finkel wrote:
> ----- Original Message -----
> > From: "Bill Schmidt" <wschmidt at linux.vnet.ibm.com>
> > To: "Hal Finkel" <hfinkel at anl.gov>
> > Cc: llvm-commits at cs.uiuc.edu
> > Sent: Monday, April 15, 2013 1:03:29 PM
> > Subject: Re: [llvm] r179500 - Fix PPC64 CR spill location for callee-saved registers
> >
> >
> >
> > On Mon, 2013-04-15 at 11:07 -0500, Hal Finkel wrote:
> > > ----- Original Message -----
> > > > From: "Bill Schmidt" <wschmidt at linux.vnet.ibm.com>
> > > > To: "Hal Finkel" <hfinkel at anl.gov>
> > > > Cc: llvm-commits at cs.uiuc.edu
> > > > Sent: Monday, April 15, 2013 9:42:43 AM
> > > > Subject: Re: [llvm] r179500 - Fix PPC64 CR spill location for
> > > > callee-saved registers
> > > >
> > > > On Mon, 2013-04-15 at 02:07 +0000, Hal Finkel wrote:
> > > > > Author: hfinkel
> > > > > Date: Sun Apr 14 21:07:05 2013
> > > > > New Revision: 179500
> > > > >
> > > > > URL: http://llvm.org/viewvc/llvm-project?rev=179500&view=rev
> > > > > Log:
> > > > > Fix PPC64 CR spill location for callee-saved registers
> > > > >
> > > > > This fixes an ABI bug for non-Darwin PPC64. For the
> > > > > callee-saved
> > > > > condition
> > > > > registers, the spill location is specified relative to the
> > > > > stack
> > > > > pointer (SP +
> > > > > 8). However, this is not relative to the SP after the new stack
> > > > > frame is
> > > > > established, but instead relative to the caller's stack pointer
> > > > > (it
> > > > > is stored
> > > > > into the linkage area of the parent's stack frame).
> > > > >
> > > > > So, like with the link register, we don't directly spill the
> > > > > CRs
> > > > > with other
> > > > > callee-saved registers, but just mark them to be spilled during
> > > > > prologue
> > > > > generation.
> > > > >
> > > > > In practice, this reverts r179457 for PPC64 (but leaves it in
> > > > > place
> > > > > for PPC32).
> > > > >
> > > > > Modified:
> > > > > llvm/trunk/lib/Target/PowerPC/PPCFrameLowering.cpp
> > > > > llvm/trunk/lib/Target/PowerPC/PPCMachineFunctionInfo.h
> > > > > llvm/trunk/test/CodeGen/PowerPC/crsave.ll
> > > > >
> > > > > Modified: llvm/trunk/lib/Target/PowerPC/PPCFrameLowering.cpp
> > > > > URL:
> > > > > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCFrameLowering.cpp?rev=179500&r1=179499&r2=179500&view=diff
> > > > > ==============================================================================
> > > > > --- llvm/trunk/lib/Target/PowerPC/PPCFrameLowering.cpp
> > > > > (original)
> > > > > +++ llvm/trunk/lib/Target/PowerPC/PPCFrameLowering.cpp Sun Apr
> > > > > 14
> > > > > 21:07:05 2013
> > > > > @@ -223,9 +223,7 @@ unsigned PPCFrameLowering::determineFram
> > > > >
> > > > > // If we are a leaf function, and use up to 224 bytes of
> > > > > stack
> > > > > space,
> > > > > // don't have a frame pointer, calls, or dynamic alloca then
> > > > > we
> > > > > do not need
> > > > > - // to adjust the stack pointer (we fit in the Red Zone).
> > > > > For
> > > > > 64-bit
> > > > > - // SVR4, we also require a stack frame if we need to spill
> > > > > the
> > > > > CR,
> > > > > - // since this spill area is addressed relative to the stack
> > > > > pointer.
> > > > > + // to adjust the stack pointer (we fit in the Red Zone).
> > > > > // The 32-bit SVR4 ABI has no Red Zone. However, it can
> > > > > still
> > > > > generate
> > > > > // stackless code if all local vars are reg-allocated.
> > > > > bool DisableRedZone = MF.getFunction()->getAttributes().
> > > > > @@ -237,9 +235,6 @@ unsigned PPCFrameLowering::determineFram
> > > > > FrameSize <= 224 && // Fits in
> > > > > red
> > > > > zone.
> > > > > !MFI->hasVarSizedObjects() && // No
> > > > > dynamic
> > > > > alloca.
> > > > > !MFI->adjustsStack() && // No
> > > > > calls.
> > > > > - !(Subtarget.isPPC64() && // No
> > > > > 64-bit
> > > > > SVR4 CRsave.
> > > > > - Subtarget.isSVR4ABI()
> > > > > - && spillsCR(MF)) &&
> > > >
> > > > Hi Hal,
> > > >
> > > > I don't understand why you removed this. This constitutes a
> > > > performance
> > > > regression. We want to use the red zone if at all possible, and
> > > > this
> > > > logic was added recently per a user request. Please fix. You
> > > > can
> > > > either reinstate the call to FuncInfo->setSpillsCR() or check
> > > > MustSaveCRs.empty() here, right?
> > >
> > > I think that you're reading this incorrectly; this is a performance
> > > improvement. I've removed one case in which the optimization was
> > > disabled. Because we don't need a stack frame in order to save the
> > > CRs, this is yet another case in which we don't need to force a
> > > stack frame. (there was a !() around that logical expression).
> >
> > Sorry, I misremembered the history here and read it too quickly.
> > However, you still can't remove the code. If you spill the CR into
> > the
> > ABI-specified location, and you don't stack a frame, you are
> > overwriting
> > the caller's CR spill area. If the caller also spilled a CR, this
> > will
> > break code. I doubt seriously that there is a test case that covers
> > this.
> >
> > I think your description of the patch may have led you into error.
> > When
> > you say "this is not relative to the SP after the new stack frame is
> > established," that isn't true. The location is 8(SP) after the SP
> > has
> > been adjusted with a stdu (as your test case confirms). The save
> > location is in the current function's frame, and hence you have to
> > stack
> > a frame if you want to spill the CR.
>
> We discussed this briefly on IRC, but to elaborate: The problem seems to be that we need to establish a new stack frame if we are not a leaf function and we save callee-saved CR registers. However, I think that non-leaf functions always establish a stack frame, but I'll double-check the logic to make sure that this is happening when necessary. Also, the same is true for saving LR, and I'll check on that as well. Regardless, this is still a separate concern from the allocation of register scavenger slots, and we should use a separate variable to keep track of it as necessary.
OK. I agree that the code generation is correct, in that it matches
existing GCC prologue/epilogue generation. Unfortunately the ABI is
poorly written on this point. Other fields that belong in the caller's
stack frame are correctly called out as being in the caller's stack
frame. For LR and CRsave, this is not specified, and the implication
appears to be that they belong in the callee's save frame. Later,
however, there is language indicating the LR is saved in the caller
frame. We can infer that CRsave should be as well.
I'll add this to the long list of things that need to be cleaned up in
the ABI.
I'm out the rest of the week but will check back periodically if there
are any other issues to discuss this week...
Thanks,
Bill
>
> However, the description of the patch is correct, and the test case tests for:
> ; PPC64: mfcr 12
> ; PPC64: stw 12, 8(1)
> ; PPC64: stdu 1, -[[AMT:[0-9]+]](1)
> ...
> which does show the store happening prior to the SP being decremented.
>
> Thanks again,
> Hal
>
> >
> > >
> > > In any case, as I recall, there are regression tests which cover
> > > the red-zone optimization, and they would have complained had I
> > > broken it. If not, please add one ;)
> > >
> > > >
> > > > > (!ALIGN_STACK || MaxAlign <= TargetAlign)) { // No
> > > > > special
> > > > > alignment.
> > > > > // No need for frame
> > > > > if (UpdateMF)
> > > > > @@ -373,6 +368,7 @@ void PPCFrameLowering::emitPrologue(Mach
> > > > > // Check if the link register (LR) must be saved.
> > > > > PPCFunctionInfo *FI = MF.getInfo<PPCFunctionInfo>();
> > > > > bool MustSaveLR = FI->mustSaveLR();
> > > > > + const SmallVector<unsigned, 3> &MustSaveCRs =
> > > > > FI->getMustSaveCRs();
> > > > > // Do we have a frame pointer for this function?
> > > > > bool HasFP = hasFP(MF);
> > > > >
> > > > > @@ -394,6 +390,13 @@ void PPCFrameLowering::emitPrologue(Mach
> > > > > if (MustSaveLR)
> > > > > BuildMI(MBB, MBBI, dl, TII.get(PPC::MFLR8), PPC::X0);
> > > > >
> > > > > + if (!MustSaveCRs.empty()) {
> > > > > + MachineInstrBuilder MIB =
> > > > > + BuildMI(MBB, MBBI, dl, TII.get(PPC::MFCR8), PPC::X12);
> > > > > + for (unsigned i = 0, e = MustSaveCRs.size(); i != e;
> > > > > ++i)
> > > > > + MIB.addReg(MustSaveCRs[i], RegState::ImplicitKill);
> > > > > + }
> > > > > +
> > > > > if (HasFP)
> > > > > BuildMI(MBB, MBBI, dl, TII.get(PPC::STD))
> > > > > .addReg(PPC::X31)
> > > > > @@ -405,6 +408,12 @@ void PPCFrameLowering::emitPrologue(Mach
> > > > > .addReg(PPC::X0)
> > > > > .addImm(LROffset / 4)
> > > > > .addReg(PPC::X1);
> > > > > +
> > > > > + if (!MustSaveCRs.empty())
> > > > > + BuildMI(MBB, MBBI, dl, TII.get(PPC::STW8))
> > > > > + .addReg(PPC::X12, getKillRegState(true))
> > > > > + .addImm(8)
> > > > > + .addReg(PPC::X1);
> > > > > } else {
> > > > > if (MustSaveLR)
> > > > > BuildMI(MBB, MBBI, dl, TII.get(PPC::MFLR), PPC::R0);
> > > > > @@ -417,6 +426,9 @@ void PPCFrameLowering::emitPrologue(Mach
> > > > > .addImm(FPOffset)
> > > > > .addReg(PPC::R1);
> > > > >
> > > > > + assert(MustSaveCRs.empty() &&
> > > > > + "Prologue CR saving supported only in 64-bit
> > > > > mode");
> > > > > +
> > > > > if (MustSaveLR)
> > > > > BuildMI(MBB, MBBI, dl, TII.get(PPC::STW))
> > > > > .addReg(PPC::R0)
> > > > > @@ -580,7 +592,7 @@ void PPCFrameLowering::emitPrologue(Mach
> > > > > // spilled CRs.
> > > > > if (Subtarget.isSVR4ABI()
> > > > > && (PPC::CR2 <= Reg && Reg <= PPC::CR4)
> > > > > - && !spillsCR(MF))
> > > > > + && MustSaveCRs.empty())
> > > > > continue;
> > > > >
> > > > > // For 64-bit SVR4 when we have spilled CRs, the spill
> > > > > location
> > > > > @@ -636,6 +648,7 @@ void PPCFrameLowering::emitEpilogue(Mach
> > > > > // Check if the link register (LR) has been saved.
> > > > > PPCFunctionInfo *FI = MF.getInfo<PPCFunctionInfo>();
> > > > > bool MustSaveLR = FI->mustSaveLR();
> > > > > + const SmallVector<unsigned, 3> &MustSaveCRs =
> > > > > FI->getMustSaveCRs();
> > > > > // Do we have a frame pointer for this function?
> > > > > bool HasFP = hasFP(MF);
> > > > >
> > > > > @@ -736,10 +749,19 @@ void PPCFrameLowering::emitEpilogue(Mach
> > > > > BuildMI(MBB, MBBI, dl, TII.get(PPC::LD), PPC::X0)
> > > > > .addImm(LROffset/4).addReg(PPC::X1);
> > > > >
> > > > > + if (!MustSaveCRs.empty())
> > > > > + BuildMI(MBB, MBBI, dl, TII.get(PPC::LWZ8), PPC::X12)
> > > > > + .addImm(8).addReg(PPC::X1);
> > > > > +
> > > > > if (HasFP)
> > > > > BuildMI(MBB, MBBI, dl, TII.get(PPC::LD), PPC::X31)
> > > > > .addImm(FPOffset/4).addReg(PPC::X1);
> > > > >
> > > > > + if (!MustSaveCRs.empty())
> > > > > + for (unsigned i = 0, e = MustSaveCRs.size(); i != e;
> > > > > ++i)
> > > > > + BuildMI(MBB, MBBI, dl, TII.get(PPC::MTCRF8),
> > > > > MustSaveCRs[i])
> > > > > + .addReg(PPC::X12, getKillRegState(i == e-1));
> > > > > +
> > > > > if (MustSaveLR)
> > > > > BuildMI(MBB, MBBI, dl,
> > > > > TII.get(PPC::MTLR8)).addReg(PPC::X0);
> > > > > } else {
> > > > > @@ -747,6 +769,9 @@ void PPCFrameLowering::emitEpilogue(Mach
> > > > > BuildMI(MBB, MBBI, dl, TII.get(PPC::LWZ), PPC::R0)
> > > > > .addImm(LROffset).addReg(PPC::R1);
> > > > >
> > > > > + assert(MustSaveCRs.empty() &&
> > > > > + "Epilogue CR restoring supported only in 64-bit
> > > > > mode");
> > > > > +
> > > > > if (HasFP)
> > > > > BuildMI(MBB, MBBI, dl, TII.get(PPC::LWZ), PPC::R31)
> > > > > .addImm(FPOffset).addReg(PPC::R1);
> > > > > @@ -1139,23 +1164,13 @@
> > > > > PPCFrameLowering::spillCalleeSavedRegist
> > > > >
> > > > > // Insert the spill to the stack frame.
> > > > > if (IsCRField) {
> > > > > - CRSpilled = true;
> > > > > - // The first time we see a CR field, store the whole CR
> > > > > into
> > > > > the
> > > > > - // save slot via GPR12 (available in the prolog for 32-
> > > > > and
> > > > > 64-bit).
> > > > > + PPCFunctionInfo *FuncInfo =
> > > > > MF->getInfo<PPCFunctionInfo>();
> > > > > if (Subtarget.isPPC64()) {
> > > > > - // 64-bit: SP+8
> > > > > - bool is31 = needsFP(*MF);
> > > > > - unsigned FP8Reg = is31 ? PPC::X31 : PPC::X1;
> > > > > - CRMIB = BuildMI(*MF, DL, TII.get(PPC::MFCR8),
> > > > > PPC::X12)
> > > > > - .addReg(Reg, RegState::ImplicitKill);
> > > > > -
> > > > > - MBB.insert(MI, CRMIB);
> > > > > - MBB.insert(MI, BuildMI(*MF, DL, TII.get(PPC::STW8))
> > > > > - .addReg(PPC::X12,
> > > > > - getKillRegState(true))
> > > > > - .addImm(8)
> > > > > - .addReg(FP8Reg));
> > > > > + // The actual spill will happen at the start of the
> > > > > prologue.
> > > > > + FuncInfo->addMustSaveCR(Reg);
> > > > > } else {
> > > > > + CRSpilled = true;
> > > > > +
> > > > > // 32-bit: FP-relative. Note that we made sure CR2-CR4 all
> > > > > have
> > > > > // the same frame index in
> > > > > PPCRegisterInfo::hasReservedSpillSlot.
> > > > > CRMIB = BuildMI(*MF, DL, TII.get(PPC::MFCR), PPC::R12)
> > > > > @@ -1167,10 +1182,6 @@ PPCFrameLowering::spillCalleeSavedRegist
> > > > > getKillRegState(true)),
> > > > > CSI[i].getFrameIdx()));
> > > > > }
> > > > > -
> > > > > - // Record that we spill the CR in this function.
> > > > > - PPCFunctionInfo *FuncInfo =
> > > > > MF->getInfo<PPCFunctionInfo>();
> > > > > - FuncInfo->setSpillsCR();
> > > >
> > > > By removing this last call, have you changed behavior anywhere
> > > > else?
> > > > The value of this field is checked several times in
> > > > PPCFrameLowering.cpp, for instance. Was this call redundant with
> > > > the
> > > > calls in PPCInstrInfo.cpp? Is the field itself redundant with
> > > > MustSaveCRs, and should that be checked instead in these places?
> > > > I
> > > > haven't tried to sort through it; I just want to be sure this has
> > > > been
> > > > thought through.
> > >
> > > The SpillsCR flag in PPCFunctionInfo was previously being used for
> > > two very different thing: One was saving callee-saved CRs, and the
> > > second was for register-pressure-based spilling. For the latter,
> > > we need extra spill slots to satisfy register scavenger
> > > requirements. On PPC64, these two things are significantly
> > > different, because the callee-saved case is handled in this
> > > special way. Now, this flag is only used for the case when the
> > > scavenger spill slots need to be allocated (which was its original
> > > purpose). I've looked at all of the other references of this
> > > variable, and I think that everything is consistent now.
> > >
> > > Thanks again,
> > > Hal
> > >
> > > >
> > > > Thanks,
> > > > Bill
> > > >
> > > > > } else {
> > > > > const TargetRegisterClass *RC =
> > > > > TRI->getMinimalPhysRegClass(Reg);
> > > > > TII.storeRegToStackSlot(MBB, MI, Reg, true,
> > > > > @@ -1192,15 +1203,10 @@ restoreCRs(bool isPPC64, bool is31,
> > > > > DebugLoc DL;
> > > > > unsigned RestoreOp, MoveReg;
> > > > >
> > > > > - if (isPPC64) {
> > > > > - // 64-bit: SP+8
> > > > > - unsigned FP8Reg = is31 ? PPC::X31 : PPC::X1;
> > > > > - MBB.insert(MI, BuildMI(*MF, DL, TII.get(PPC::LWZ8),
> > > > > PPC::X12)
> > > > > - .addImm(8)
> > > > > - .addReg(FP8Reg));
> > > > > - RestoreOp = PPC::MTCRF8;
> > > > > - MoveReg = PPC::X12;
> > > > > - } else {
> > > > > + if (isPPC64)
> > > > > + // This is handled during epilogue generation.
> > > > > + return;
> > > > > + else {
> > > > > // 32-bit: FP-relative
> > > > > MBB.insert(MI, addFrameReference(BuildMI(*MF, DL,
> > > > > TII.get(PPC::LWZ),
> > > > > PPC::R12),
> > > > >
> > > > > Modified:
> > > > > llvm/trunk/lib/Target/PowerPC/PPCMachineFunctionInfo.h
> > > > > URL:
> > > > > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCMachineFunctionInfo.h?rev=179500&r1=179499&r2=179500&view=diff
> > > > > ==============================================================================
> > > > > --- llvm/trunk/lib/Target/PowerPC/PPCMachineFunctionInfo.h
> > > > > (original)
> > > > > +++ llvm/trunk/lib/Target/PowerPC/PPCMachineFunctionInfo.h Sun
> > > > > Apr
> > > > > 14 21:07:05 2013
> > > > > @@ -84,6 +84,11 @@ class PPCFunctionInfo : public MachineFu
> > > > > /// CRSpillFrameIndex - FrameIndex for CR spill slot for
> > > > > 32-bit
> > > > > SVR4.
> > > > > int CRSpillFrameIndex;
> > > > >
> > > > > + /// If any of CR[2-4] need to be saved in the prologue and
> > > > > restored in the
> > > > > + /// epilogue then they are added to this array. This is used
> > > > > for
> > > > > the
> > > > > + /// 64-bit SVR4 ABI.
> > > > > + SmallVector<unsigned, 3> MustSaveCRs;
> > > > > +
> > > > > public:
> > > > > explicit PPCFunctionInfo(MachineFunction &MF)
> > > > > : FramePointerSaveIndex(0),
> > > > > @@ -154,6 +159,10 @@ public:
> > > > >
> > > > > int getCRSpillFrameIndex() const { return CRSpillFrameIndex;
> > > > > }
> > > > > void setCRSpillFrameIndex(int idx) { CRSpillFrameIndex =
> > > > > idx; }
> > > > > +
> > > > > + const SmallVector<unsigned, 3> &
> > > > > + getMustSaveCRs() const { return MustSaveCRs; }
> > > > > + void addMustSaveCR(unsigned Reg) {
> > > > > MustSaveCRs.push_back(Reg); }
> > > > > };
> > > > >
> > > > > } // end of namespace llvm
> > > > >
> > > > > Modified: llvm/trunk/test/CodeGen/PowerPC/crsave.ll
> > > > > URL:
> > > > > http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/PowerPC/crsave.ll?rev=179500&r1=179499&r2=179500&view=diff
> > > > > ==============================================================================
> > > > > --- llvm/trunk/test/CodeGen/PowerPC/crsave.ll (original)
> > > > > +++ llvm/trunk/test/CodeGen/PowerPC/crsave.ll Sun Apr 14
> > > > > 21:07:05
> > > > > 2013
> > > > > @@ -1,5 +1,4 @@
> > > > > ; RUN: llc -O0 -disable-fp-elim
> > > > > -mtriple=powerpc-unknown-linux-gnu
> > > > > < %s | FileCheck %s -check-prefix=PPC32
> > > > > -; RUN: llc -O0 -disable-fp-elim
> > > > > -mtriple=powerpc64-unknown-linux-gnu < %s | FileCheck %s
> > > > > -check-prefix=PPC64-FP
> > > > > ; RUN: llc -O0 -mtriple=powerpc64-unknown-linux-gnu < %s |
> > > > > FileCheck %s -check-prefix=PPC64
> > > > >
> > > > > declare void @foo()
> > > > > @@ -20,14 +19,11 @@ entry:
> > > > > ; PPC32-NEXT: mtcrf 32, 12
> > > > >
> > > > > ; PPC64: mfcr 12
> > > > > -; PPC64-NEXT: stw 12, 8(1)
> > > > > +; PPC64: stw 12, 8(1)
> > > > > +; PPC64: stdu 1, -[[AMT:[0-9]+]](1)
> > > > > +; PPC64: addi 1, 1, [[AMT]]
> > > > > ; PPC64: lwz 12, 8(1)
> > > > > -; PPC64-NEXT: mtcrf 32, 12
> > > > > -
> > > > > -; PPC64-FP: mfcr 12
> > > > > -; PPC64-FP-NEXT: stw 12, 8(31)
> > > > > -; PPC64-FP: lwz 12, 8(31)
> > > > > -; PPC64-FP-NEXT: mtcrf 32, 12
> > > > > +; PPC64: mtcrf 32, 12
> > > > >
> > > > > define i32 @test_cr234() nounwind {
> > > > > entry:
> > > > > @@ -47,16 +43,11 @@ entry:
> > > > > ; PPC32-NEXT: mtcrf 8, 12
> > > > >
> > > > > ; PPC64: mfcr 12
> > > > > -; PPC64-NEXT: stw 12, 8(1)
> > > > > +; PPC64: stw 12, 8(1)
> > > > > +; PPC64: stdu 1, -[[AMT:[0-9]+]](1)
> > > > > +; PPC64: addi 1, 1, [[AMT]]
> > > > > ; PPC64: lwz 12, 8(1)
> > > > > -; PPC64-NEXT: mtcrf 32, 12
> > > > > -; PPC64-NEXT: mtcrf 16, 12
> > > > > -; PPC64-NEXT: mtcrf 8, 12
> > > > > -
> > > > > -; PPC64-FP: mfcr 12
> > > > > -; PPC64-FP-NEXT: stw 12, 8(31)
> > > > > -; PPC64-FP: lwz 12, 8(31)
> > > > > -; PPC64-FP-NEXT: mtcrf 32, 12
> > > > > -; PPC64-FP-NEXT: mtcrf 16, 12
> > > > > -; PPC64-FP-NEXT: mtcrf 8, 12
> > > > > +; PPC64: mtcrf 32, 12
> > > > > +; PPC64: mtcrf 16, 12
> > > > > +; PPC64: mtcrf 8, 12
> > > > >
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > llvm-commits mailing list
> > > > > llvm-commits at cs.uiuc.edu
> > > > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> > > > >
> > > >
> > > >
> > >
> >
> >
>
More information about the llvm-commits
mailing list