Skip to content

[MIPS]: Rework atomic max/min expand for subword #89575

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

Merged
merged 1 commit into from
Apr 22, 2024
Merged

[MIPS]: Rework atomic max/min expand for subword #89575

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
114 changes: 54 additions & 60 deletions llvm/lib/Target/Mips/MipsExpandPseudo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,7 @@ bool MipsExpandPseudo::expandAtomicBinOpSubword(
bool IsMin = false;
bool IsMax = false;
bool IsUnsigned = false;
bool DestOK = false;

unsigned Opcode = 0;
switch (I->getOpcode()) {
Expand Down Expand Up @@ -388,6 +389,7 @@ bool MipsExpandPseudo::expandAtomicBinOpSubword(
Opcode = Mips::XOR;
break;
case Mips::ATOMIC_LOAD_UMIN_I8_POSTRA:
SEOp = Mips::SEB;
IsUnsigned = true;
IsMin = true;
break;
Expand All @@ -403,6 +405,7 @@ bool MipsExpandPseudo::expandAtomicBinOpSubword(
IsMin = true;
break;
case Mips::ATOMIC_LOAD_UMAX_I8_POSTRA:
SEOp = Mips::SEB;
IsUnsigned = true;
IsMax = true;
break;
Expand Down Expand Up @@ -473,48 +476,38 @@ bool MipsExpandPseudo::expandAtomicBinOpSubword(
unsigned SELOldVal = IsMax ? SELEQZ : SELNEZ;
unsigned MOVIncr = IsMax ? MOVN : MOVZ;

// For little endian we need to clear uninterested bits.
if (STI->isLittle()) {
if (!IsUnsigned) {
BuildMI(loopMBB, DL, TII->get(Mips::SRAV), OldVal)
.addReg(OldVal)
.addReg(ShiftAmnt);
BuildMI(loopMBB, DL, TII->get(Mips::SRAV), Incr)
.addReg(Incr)
Copy link

Choose a reason for hiding this comment

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

I have noticed that sign-extension of Incr subword is removed. Why is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For RegState::Kill, I noticed that it is also used when dest reg is same with one of src regs, not only in MIPS code.
In fact I am not sure about its really meanings.

Incr is an argument of this function passed by register $5. We can assume that it has been well sign-/zero- extended.

Copy link
Contributor

Choose a reason for hiding this comment

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

@wzssyqa Do I need to remove the RegState::Kill of StoreVal? The current test has not found any problems related to this. But I once encountered a problem related to RegState::Kill when I create a new reg like Scratch in function MachineBasicBlock *MipsTargetLowering::emitAtomicBinaryPartword and use it in function bool MipsExpandPseudo::expandAtomicBinOpSubword to replace OldVal, and when the code is executed to slt, an error is reported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems it's safe to use RegState::Kill for something like a <- op(a).
I guess it is even more friendly to register allocator, Since the register allocator can even use different hardware register for before/after this RegState::Kill note.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, now it is OK for a <- op(a), the scenario where my lab error reported is b <- op(a, RegState::Kill) + op(a).

.addReg(ShiftAmnt);
if (STI->hasMips32r2()) {
BuildMI(loopMBB, DL, TII->get(SEOp), OldVal).addReg(OldVal);
BuildMI(loopMBB, DL, TII->get(SEOp), Incr).addReg(Incr);
} else {
const unsigned ShiftImm = SEOp == Mips::SEH ? 16 : 24;
BuildMI(loopMBB, DL, TII->get(Mips::SLL), OldVal)
.addReg(OldVal, RegState::Kill)
.addImm(ShiftImm);
BuildMI(loopMBB, DL, TII->get(Mips::SRA), OldVal)
.addReg(OldVal, RegState::Kill)
.addImm(ShiftImm);
BuildMI(loopMBB, DL, TII->get(Mips::SLL), Incr)
.addReg(Incr, RegState::Kill)
.addImm(ShiftImm);
BuildMI(loopMBB, DL, TII->get(Mips::SRA), Incr)
.addReg(Incr, RegState::Kill)
.addImm(ShiftImm);
}
} else {
// and OldVal, OldVal, Mask
// and Incr, Incr, Mask
BuildMI(loopMBB, DL, TII->get(Mips::AND), OldVal)
.addReg(OldVal)
.addReg(Mask);
BuildMI(loopMBB, DL, TII->get(Mips::AND), Incr)
.addReg(Incr)
.addReg(Mask);
}
BuildMI(loopMBB, DL, TII->get(Mips::SRAV), StoreVal)
.addReg(OldVal)
.addReg(ShiftAmnt);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: ShiftAmnt may be zero. In this case, the high-part of OldVal will be keeped.
If we use that value to compare, we will get wrong result.

if (STI->hasMips32r2() && !IsUnsigned) {
BuildMI(loopMBB, DL, TII->get(SEOp), StoreVal).addReg(StoreVal);
} else if (STI->hasMips32r2() && IsUnsigned) {
const unsigned OpMask = SEOp == Mips::SEH ? 0xffff : 0xff;
BuildMI(loopMBB, DL, TII->get(Mips::ANDi), StoreVal)
.addReg(StoreVal)
.addImm(OpMask);
} else {
const unsigned ShiftImm = SEOp == Mips::SEH ? 16 : 24;
const unsigned SROp = IsUnsigned ? Mips::SRL : Mips::SRA;
BuildMI(loopMBB, DL, TII->get(Mips::SLL), StoreVal)
.addReg(StoreVal, RegState::Kill)
.addImm(ShiftImm);
BuildMI(loopMBB, DL, TII->get(SROp), StoreVal)
.addReg(StoreVal, RegState::Kill)
.addImm(ShiftImm);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yingopq You talked about here?
Note: the unsigned of SLTU is about word-size. Here we use it to compare subword.
If we use it to compare signed subword, we need to be sure that the value is sign-extend.
If we use it to compare unsigned subword, we need to be sure that the value is zero-extend.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why unsigned have else if and else cases with STI->hasMips32r2()?

Copy link
Contributor Author

@wzssyqa wzssyqa Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You are right. ANDi is available for all ISA revisions.
So we can

if (IsUnsigned)
   ANDi
else if (STI->hasMips32r2())
   SEH/SEB
else
   SLL
   SRA

So that we can save an INSN for Unsigned+pre-R2.
Can you file a new PR for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I submit and thank you for helping to solve this problem and explaining it carefully.

Copy link

Choose a reason for hiding this comment

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

One question: is it valid to specify RegState::Kill when this isn't the last usage of the register StoreVal?

}
// unsigned: sltu Scratch4, oldVal, Incr
// signed: slt Scratch4, oldVal, Incr
BuildMI(loopMBB, DL, TII->get(Mips::OR), Dest)
.addReg(Mips::ZERO)
.addReg(StoreVal);
DestOK = true;
BuildMI(loopMBB, DL, TII->get(Mips::SLLV), StoreVal)
.addReg(StoreVal)
.addReg(ShiftAmnt);

// unsigned: sltu Scratch4, StoreVal, Incr
// signed: slt Scratch4, StoreVal, Incr
BuildMI(loopMBB, DL, TII->get(SLTScratch4), Scratch4)
.addReg(OldVal)
.addReg(StoreVal)
.addReg(Incr);

if (STI->hasMips64r6() || STI->hasMips32r6()) {
Expand All @@ -525,7 +518,7 @@ bool MipsExpandPseudo::expandAtomicBinOpSubword(
// seleqz Scratch4, Incr, Scratch4
// or BinOpRes, BinOpRes, Scratch4
BuildMI(loopMBB, DL, TII->get(SELOldVal), BinOpRes)
.addReg(OldVal)
.addReg(StoreVal)
.addReg(Scratch4);
BuildMI(loopMBB, DL, TII->get(SELIncr), Scratch4)
.addReg(Incr)
Expand All @@ -534,12 +527,12 @@ bool MipsExpandPseudo::expandAtomicBinOpSubword(
.addReg(BinOpRes)
.addReg(Scratch4);
} else {
// max: move BinOpRes, OldVal
// max: move BinOpRes, StoreVal
// movn BinOpRes, Incr, Scratch4, BinOpRes
// min: move BinOpRes, OldVal
// min: move BinOpRes, StoreVal
// movz BinOpRes, Incr, Scratch4, BinOpRes
BuildMI(loopMBB, DL, TII->get(OR), BinOpRes)
.addReg(OldVal)
.addReg(StoreVal)
.addReg(Mips::ZERO);
BuildMI(loopMBB, DL, TII->get(MOVIncr), BinOpRes)
.addReg(Incr)
Expand Down Expand Up @@ -586,23 +579,24 @@ bool MipsExpandPseudo::expandAtomicBinOpSubword(
// srl srlres,maskedoldval1,shiftamt
// sign_extend dest,srlres

sinkMBB->addSuccessor(exitMBB, BranchProbability::getOne());

BuildMI(sinkMBB, DL, TII->get(Mips::AND), Dest)
.addReg(OldVal).addReg(Mask);
BuildMI(sinkMBB, DL, TII->get(Mips::SRLV), Dest)
.addReg(Dest).addReg(ShiftAmnt);
if (!DestOK) {
sinkMBB->addSuccessor(exitMBB, BranchProbability::getOne());
BuildMI(sinkMBB, DL, TII->get(Mips::AND), Dest).addReg(OldVal).addReg(Mask);
BuildMI(sinkMBB, DL, TII->get(Mips::SRLV), Dest)
.addReg(Dest)
.addReg(ShiftAmnt);

if (STI->hasMips32r2()) {
BuildMI(sinkMBB, DL, TII->get(SEOp), Dest).addReg(Dest);
} else {
const unsigned ShiftImm = SEOp == Mips::SEH ? 16 : 24;
BuildMI(sinkMBB, DL, TII->get(Mips::SLL), Dest)
.addReg(Dest, RegState::Kill)
.addImm(ShiftImm);
BuildMI(sinkMBB, DL, TII->get(Mips::SRA), Dest)
.addReg(Dest, RegState::Kill)
.addImm(ShiftImm);
if (STI->hasMips32r2()) {
BuildMI(sinkMBB, DL, TII->get(SEOp), Dest).addReg(Dest);
} else {
const unsigned ShiftImm = SEOp == Mips::SEH ? 16 : 24;
BuildMI(sinkMBB, DL, TII->get(Mips::SLL), Dest)
.addReg(Dest, RegState::Kill)
.addImm(ShiftImm);
BuildMI(sinkMBB, DL, TII->get(Mips::SRA), Dest)
.addReg(Dest, RegState::Kill)
.addImm(ShiftImm);
}
}

LivePhysRegs LiveRegs;
Expand Down
Loading
Loading