Skip to content

[PPC]Optimize zeroing accumulator and spilling instructions into simple instructions #96094

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

[PPC]Optimize zeroing accumulator and spilling instructions into simple instructions #96094

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
26beee4
first commit of replace XXSETACCZ
diggerlin Jun 18, 2024
2f8614a
deal with not adjacent instruction
diggerlin Aug 9, 2024
1bca20b
Merge branch 'llvm:main' into digger/XXSETACCZ
diggerlin Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,93 @@ static bool hasPCRelativeForm(MachineInstr &Use) {
MachineFunctionProperties::Property::NoVRegs);
}

// The funtion will simply the zeroing accumulator and spilling instrcutions
// into simple xxlxor and spilling instrcuctions.
// From:
// setaccz acci
// xxmfacc acci
// stxv vsr(i*4+0), D(1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does D(1) mean? 1 is for R1? could we use another name like Base? And we don't check the offsets in the implementation, so maybe the D/D-16/D-32/D-48 is also not necessary?

// stxv vsr(i*4+1), D-16(1)
// stxv vsr(i*4+2), D-32(1)
// stxv vsr(i*4+3), D-48(1)

// To:
// xxlxor vsr(i*4), 0, 0
// stxv vsr(i*4), D(1)
// stxv vsr(i*4), D-16(1)
// stxv vsr(i*4), D-32(1)
// stxv vsr(i*4), D-48(1)
bool
OptimizeZeroingAccumulatorSpilling(MachineBasicBlock &MBB,
const TargetRegisterInfo *TRI) const {
bool changed = false;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for variables, first letter is capitalized.

for (auto BBI = MBB.instr_begin(); BBI != MBB.instr_end(); ++BBI) {
if (BBI->getOpcode() != PPC::XXSETACCZ)
continue;

Register ACCZReg = BBI->getOperand(0).getReg();

DenseSet<MachineInstr *> InstrsToErase;
InstrsToErase.insert(&*BBI++);

Copy link
Collaborator

@chenzheng1030 chenzheng1030 Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not look good. We need a more generic handling for all cases. For now, this is targeted for a specific case in the comment. Compiler scheduling may change the order of these instructions.

  • it is not necessary there must be 4 stores...
  • limiting the case that XXSETACCZ and XXMFACC must be adjacent is not a good idea...
  • limitting the case that the four stores must be followed XXMFACC is also not good.
  • The loop structure also seems not able to handle case
    // setaccz acci
    // xxmfacc acci

    // setaccz acci2
    // xxmfacc acci2

    // stxv vsr(i*4+0), D(1)
    // stxv vsr(i*4+1), D-16(1)
    // stxv vsr(i*4+2), D-32(1)
    // stxv vsr(i*4+3), D-48(1)

    // stxv vsr(i2*4+0), D(2)
    // stxv vsr(i2*4+1), D-16(2)
    // stxv vsr(i2*4+2), D-32(2)
    // stxv vsr(i2*4+3), D-48(2)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.
We want this to be a general solution. I'm not sure that the approach of putting this so late in the pre-emit peephole is a good idea. When are these stores generated? Is this a spill? Is it just a store of a zeroed ACC register?

It may be easier to fix this the first time when the stores are generated because then the instruction scheduling won't be a problem and we just generate the instructions we want.

Copy link
Contributor Author

@diggerlin diggerlin Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a accumulator register spill, there is another solution to check whether there is XXSETACCZ instruction in the function PPCRegisterInfo::lowerACCSpilling()

if there is the XXSETACCZ instruction , we do not do
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp#L1336
BuildMI(MBB, II, DL, TII.get(PPC::XXMFACC), SrcReg).addReg(SrcReg);

we change the code

BuildMI(MBB, II, DL, TII.get(PPC::XXMFACC), SrcReg).addReg(SrcReg)

to something like

BuildMI(MBB, II, DL, TII->get(PPC::XXLXOR), Reg)

And in the function static void spillRegPairs change

addFrameReference(BuildMI(MBB, II, DL, TII.get(PPC::STXV))
                        .addReg(Reg + i, getKillRegState(IsKilled)),
                    FrameIndex, Offset);

to

addFrameReference(BuildMI(MBB, II, DL, TII.get(PPC::STXV))
                      .addReg(Reg , getKillRegState(IsKilled)),
                  FrameIndex, Offset);

but I do not think it is good place to put optimize in the function PPCRegisterInfo::lowerACCSpilling (the function is for spill functionality )

Copy link
Contributor Author

@diggerlin diggerlin Aug 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not necessary there must be 4 stores...

the patch is accumulator spill, it must has 4 stores.

limiting the case that XXSETACCZ and XXMFACC must be adjacent is not a good idea...

OK, I will deal the XXSETACCZ and XXMFACC are not adjacent.

limitting the case that the four stores must be followed XXMFACC is also not good.

the patch is accumulator spill, in the function PPCRegisterInfo::lowerACCSpilling. the XXMFACC is followed by
4 store, but I can deal with XXMFACC is not followed by 4 store any ways.

The loop structure also seems not able to handle case

I will deal with these case.

if (BBI->getOpcode() != PPC::XXMFACC) {
--BBI;
continue;
}

Register ACCWReg = BBI->getOperand(0).getReg();

if (ACCWReg != ACCZReg)
continue;

auto XXMFACCInstr = BBI;
InstrsToErase.insert(&*BBI++);

Register VSLRegBase = (ACCWReg - PPC::ACC0) * 4 + PPC::VSL0;
bool isVSLRegBaseKilled = false;
for (unsigned InstrCount = 0; InstrCount < 4; ++InstrCount, ++BBI) {
if (BBI->getOpcode() == PPC::STXV) {
Register Reg0 = BBI->getOperand(0).getReg();
// If the VSLRegBase Register is killed, we put the kill in the
// last STXV instruction.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a FIXME here that we may need to update killed flag for other vsr as well.

if (Reg0 == VSLRegBase && BBI->getOperand(0).isKill())
isVSLRegBaseKilled = true;
if (Reg0 < VSLRegBase || Reg0 > VSLRegBase + 3)
continue;
} else {
--BBI;
continue;
}
}

BBI = XXMFACCInstr;
BBI++;
for (unsigned InstrCount = 0; InstrCount < 4; ++InstrCount, ++BBI) {
Register VSLiReg = BBI->getOperand(0).getReg();
BBI->substituteRegister(VSLiReg, VSLRegBase, 0, *TRI);
BBI->getOperand(0).setIsKill(false);
}

if (isVSLRegBaseKilled)
(--BBI)->getOperand(0).setIsKill(true);

DebugLoc DL = XXMFACCInstr->getDebugLoc();
const PPCInstrInfo *TII = XXMFACCInstr->getMF()
->getSubtarget<PPCSubtarget>()
.getInstrInfo();

BuildMI(MBB, &*XXMFACCInstr, DL, TII->get(PPC::XXLXOR), VSLRegBase)
.addReg(VSLRegBase,RegState::Undef)
.addReg(VSLRegBase,RegState::Undef);

for (MachineInstr *MI : InstrsToErase)
MI->eraseFromParent();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can put this function outside of the main loop? i.e., we collect all the unneeded instructions and remove them all just before this function returns.


changed |= true;
}
return changed;
}

// This function removes any redundant load immediates. It has two level
// loops - The outer loop finds the load immediates BBI that could be used
// to replace following redundancy. The inner loop scans instructions that
Expand Down Expand Up @@ -466,6 +553,7 @@ static bool hasPCRelativeForm(MachineInstr &Use) {
Changed |= removeRedundantLIs(MBB, TRI);
Changed |= addLinkerOpt(MBB, TRI);
Changed |= removeAccPrimeUnprime(MBB);
Changed |= OptimizeZeroingAccumulatorSpilling(MBB, TRI);
for (MachineInstr &MI : MBB) {
unsigned Opc = MI.getOpcode();
if (Opc == PPC::UNENCODED_NOP) {
Expand Down
36 changes: 16 additions & 20 deletions llvm/test/CodeGen/PowerPC/mma-intrinsics.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,22 +115,20 @@ declare <512 x i1> @llvm.ppc.mma.xxsetaccz()
define void @int_xxsetaccz(ptr %ptr) {
; CHECK-LABEL: int_xxsetaccz:
; CHECK: # %bb.0: # %entry
; CHECK-NEXT: xxsetaccz acc0
; CHECK-NEXT: xxmfacc acc0
; CHECK-NEXT: xxlxor vs0, vs0, vs0
; CHECK-NEXT: stxv vs0, 48(r3)
; CHECK-NEXT: stxv vs1, 32(r3)
; CHECK-NEXT: stxv vs2, 16(r3)
; CHECK-NEXT: stxv vs3, 0(r3)
; CHECK-NEXT: stxv vs0, 32(r3)
; CHECK-NEXT: stxv vs0, 16(r3)
; CHECK-NEXT: stxv vs0, 0(r3)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this patch, the case at line 50 seems like an existing bug...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry that I can not got the comment, can you explain more detail ?

; CHECK-NEXT: blr
;
; CHECK-BE-LABEL: int_xxsetaccz:
; CHECK-BE: # %bb.0: # %entry
; CHECK-BE-NEXT: xxsetaccz acc0
; CHECK-BE-NEXT: xxmfacc acc0
; CHECK-BE-NEXT: stxv vs1, 16(r3)
; CHECK-BE-NEXT: xxlxor vs0, vs0, vs0
; CHECK-BE-NEXT: stxv vs0, 16(r3)
; CHECK-BE-NEXT: stxv vs0, 0(r3)
; CHECK-BE-NEXT: stxv vs3, 48(r3)
; CHECK-BE-NEXT: stxv vs2, 32(r3)
; CHECK-BE-NEXT: stxv vs0, 48(r3)
; CHECK-BE-NEXT: stxv vs0, 32(r3)
; CHECK-BE-NEXT: blr
entry:
%0 = tail call <512 x i1> @llvm.ppc.mma.xxsetaccz()
Expand All @@ -143,22 +141,20 @@ declare { <16 x i8>, <16 x i8>, <16 x i8>, <16 x i8> } @llvm.ppc.mma.disassemble
define void @disass_acc(ptr %ptr1, ptr %ptr2, ptr %ptr3, ptr %ptr4) {
; CHECK-LABEL: disass_acc:
; CHECK: # %bb.0: # %entry
; CHECK-NEXT: xxsetaccz acc0
; CHECK-NEXT: xxmfacc acc0
; CHECK-NEXT: stxv vs3, 0(r3)
; CHECK-NEXT: stxv vs2, 0(r4)
; CHECK-NEXT: stxv vs1, 0(r5)
; CHECK-NEXT: xxlxor vs0, vs0, vs0
; CHECK-NEXT: stxv vs0, 0(r3)
; CHECK-NEXT: stxv vs0, 0(r4)
; CHECK-NEXT: stxv vs0, 0(r5)
; CHECK-NEXT: stxv vs0, 0(r6)
; CHECK-NEXT: blr
;
; CHECK-BE-LABEL: disass_acc:
; CHECK-BE: # %bb.0: # %entry
; CHECK-BE-NEXT: xxsetaccz acc0
; CHECK-BE-NEXT: xxmfacc acc0
; CHECK-BE-NEXT: xxlxor vs0, vs0, vs0
; CHECK-BE-NEXT: stxv vs0, 0(r3)
; CHECK-BE-NEXT: stxv vs1, 0(r4)
; CHECK-BE-NEXT: stxv vs2, 0(r5)
; CHECK-BE-NEXT: stxv vs3, 0(r6)
; CHECK-BE-NEXT: stxv vs0, 0(r4)
; CHECK-BE-NEXT: stxv vs0, 0(r5)
; CHECK-BE-NEXT: stxv vs0, 0(r6)
; CHECK-BE-NEXT: blr
entry:
%0 = tail call <512 x i1> @llvm.ppc.mma.xxsetaccz()
Expand Down