[MIPS]: Rework atomic max/min expand for subword #89575
Conversation
|
See also the talk in: #89246 |
|
How to test: |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Thanks for @jdmitrovic-syrmia find this problem. |
The current code is so buggy: it can work for few cases.
The problems include:
1. ll/sc works on a whole word, while other parts other than we rmw
are dropped.
2. The oprands are not well zero-extended for unsigned ops.
3. It doesn't work for big-endian, as the postion of subword differs
with little endian.
|
@wzssyqa Why are there still two cases for unsigned? I don’t quite understand. Thanks! |
| .addImm(ShiftImm); | ||
| BuildMI(loopMBB, DL, TII->get(SROp), StoreVal) | ||
| .addReg(StoreVal, RegState::Kill) | ||
| .addImm(ShiftImm); |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Why unsigned have else if and else cases with STI->hasMips32r2()?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
OK, I submit and thank you for helping to solve this problem and explaining it carefully.
There was a problem hiding this comment.
One question: is it valid to specify RegState::Kill when this isn't the last usage of the register StoreVal?
| } | ||
| BuildMI(loopMBB, DL, TII->get(Mips::SRAV), StoreVal) | ||
| .addReg(OldVal) | ||
| .addReg(ShiftAmnt); |
There was a problem hiding this comment.
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.
| .addImm(ShiftImm); | ||
| BuildMI(loopMBB, DL, TII->get(SROp), StoreVal) | ||
| .addReg(StoreVal, RegState::Kill) | ||
| .addImm(ShiftImm); |
There was a problem hiding this comment.
One question: is it valid to specify RegState::Kill when this isn't the last usage of the register StoreVal?
| .addReg(OldVal) | ||
| .addReg(ShiftAmnt); | ||
| BuildMI(loopMBB, DL, TII->get(Mips::SRAV), Incr) | ||
| .addReg(Incr) |
There was a problem hiding this comment.
I have noticed that sign-extension of Incr subword is removed. Why is that?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, now it is OK for a <- op(a), the scenario where my lab error reported is b <- op(a, RegState::Kill) + op(a).
The current code is so buggy: it can work for few cases. The problems include:
And in fact, we can set the return value correct in ll/sc scope, so we can skip the sinkMBB.