This patch fixes an issue with storing/restoring pointers used by SC/LL instructions when N64 code is generated.
In case of N64 ABI sd/ld instructons should be used instead of sw/lw.
MipsTargetLowering::emitAtomicCmpSwapPartword is modified to use MVT::i64 (instead of MVT::i32) when creating virtual registers if code for N64 is generated.
Details
Diff Detail
Event Timeline
Please upload a diff with full context next time if you don't use arcanist (see http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).
We should choose the correct instruction opcodes inside the function emitAtomicCmpSwapPartword() according to the register class that we want to use (see for example the code in function emitAtomicCmpSwap()).
lib/Target/Mips/MipsISelLowering.cpp | ||
---|---|---|
1415 | We can use the ternary operator inside the getRegClassFor in order to save space. Also, make sure to clang-format the code. | |
test/CodeGen/Mips/at1.ll | ||
1 | You use %s twice here. We should pick only one method for passing the code to llc. |
lib/Target/Mips/MipsISelLowering.cpp | ||
---|---|---|
1477–1502 | We should specify the correct instructions, based on the register class, to every TII->get() in this function. For example, you could use something like that: unsigned NOR = Subtarget.isGP64bit() ? Mips::NOR64 : Mips::NOR; for every opcode we want to use. | |
test/CodeGen/Mips/at.ll | ||
1–19 ↗ | (On Diff #37471) | The test seems fine but could use a more meaningful filename, eg. test-atomic-cmp-swap.ll or something similar? |
In the method emitAtomicCmpPartword, when condition Subtaget.isGP64() is satisfied, there are included 64bit versions of atithmetic instructions.
Most of the comments are nits, but the comment about the N32 case is important and potentially tricky to fix.
lib/Target/Mips/MipsISelLowering.cpp | ||
---|---|---|
1407–1411 | Variables should start with a capital letter. | |
1408–1411 | These decision should be based on the ABI's integer/pointer width rather than the CPU capabilities. Also, be careful about the difference between a pointer and an integer value on N32. N32's pointers need to use the 32-bit operations even though the registers holding them are 64-bit. | |
1476 | You can use ABI.GetZeroReg() to pick the right zero register Just for completeness, pointers can use ABI.GetNullPtr() | |
1486 | Formatting clang-format-diff.py can format the lines mentioned in the patch automatically. If you use it, make sure you give it zero lines of context so it doesn't format nearby code too. | |
1488 | Space before '?'. Similarly below | |
test/CodeGen/Mips/atomicCmpSwapPW.ll | ||
2 ↗ | (On Diff #38645) | Could you add a comment explaining why this -implicit-check-not is the only check? As far as I can see you're checking that we don't spill/reload using sw/lw. |
Arithmetic instructions that are used are selected according to ABI's integer/pointer width.
The latest patch only deals with part of the problem. I've commented on the first few pointer/integer inconsistencies but there are more.
lib/Target/Mips/MipsISelLowering.cpp | ||
---|---|---|
1413 | Nit: For consistency please use 'IsABIN64'. It will be confusing to use '64' and 'N64' interchangably. Or preferably, refer to the pointer size instead to make it easier to keep track of the difference between integer and pointer sizes. | |
1424–1440 | Some of these should pick i64 for 64-bit integers (N32/N64), others should pick i64 for 64-bit pointers (N64 only). | |
1480 | This should pick between ADDiu and DADDiu depending on whether we have 32/64 bit pointers (at first it looks like an integer but it will be combined with a pointer in the next insn). You'll need to switch to GetNullPtr() too. | |
1487 | SLL/DSLL depending on pointer size | |
1492 | SLL/DSLL depending on pointer size | |
1494–1495 | IsABI64 and GetZeroReg() do not agree with eachother. | |
1496 | SLL/DSLLV based on pointer size (I think) | |
1498–1499 | IsABI64 and GetZeroReg() do not agree with eachother. | |
1502 | SLLV/DSLLV. It's not immediately obvious whether this is integer or pointer | |
1506 | SLLV/DSLLV. It's not immediately obvious whether this is integer or pointer |
Daniel,
could you please give me some opinion about the latest changes in this patch, is this ok?
test/CodeGen/Mips/atomicCmpSwapPW.ll | ||
---|---|---|
2 ↗ | (On Diff #38645) | I used -implicit-check-not just for lw, because after these changes in code llc doesn't emit any lw, but in one line emits sw instruction(which is correct, but -implicit-check-not sw will cause test fail). |
lib/Target/Mips/MipsISelLowering.cpp | ||
---|---|---|
1412–1413 | Use ABI.ArePtrs64bit() and ABI.AreGprs64bit() instead. | |
1496–1498 | One path considers ShiftAmt to be pointer-sized, the other integer-sized. This inconsistency needs to be fixed. With that fixed, the shift left is the same on both paths. Given that PtrLSB2 is never used after this, it's possible to merge the common parts like this: if (!Subtarget.isLittle()) { unsigned Off = RegInfo.createVirtualRegister(RCi); BuildMI(BB, DL, TII->get(ArePtrs64bit ? Mips::XORi64 : Mips::XORi), Off).addReg(PtrLSB2).addImm((Size == 1) ? 3 : 2); PtrLSB2 = Off; } BuildMI(BB, DL, TII->get(ArePtrs64bit ? Mips::DSLL : Mips::SLL), ShiftAmt).addReg(PtrLSB2).addImm(3); | |
1496–1499 | We still have some type inconsistencies here and there are two ways of solving this. The first is consistent with the code generator as it is today, the second is more correct for the hardware (and the code generator should be changed to that approach). MaskLSB2 should be pointer-sized since it is used as a mask for AlignedAddr which is a pointer. It follows that the assignment to MaskLSB2 must therefore use ABI.GetNullPtr() instead of ABI.GetZeroReg(). Similarly, PtrLSB2, Off, and ShiftAmt must be pointer-sized in this approach. However, it's a little more correct to promote 'Ptr' to be GPR-sized since there is no 32-bit bitwise AND on MIPS64 (the AND opcode always operates on GPR-width values). If you take this approach then MaskLSB2, PtrLSB2, and Off must be GPR-sized and you must truncate PtrLSB2/Off before using it as an operand for the shift that results in ShiftAmt. | |
test/CodeGen/Mips/atomicCmpSwapPW.ll | ||
3 ↗ | (On Diff #39550) | I meant a comment in the test file. I've just noticed that this test passes without your change because the code generator selects an ld and not an lw. I don't think this test covers the code change. |
8–11 ↗ | (On Diff #39550) | The personality, %exn.slot, and %ehselector.slot don't seem relevant to the test. Are they really needed? |
AFAIU, the original problem has been fixed with http://reviews.llvm.org/D14397 , so we can abandon this change and close the issue.
This minimal test case:
define {i16, i1} @foo(i16* %addr, i16 signext %r, i16 zeroext %new) { %res = cmpxchg i16* %addr, i16 %r, i16 %new seq_cst seq_cst ret {i16, i1} %res }
compiled for mips64r6 (big endian) with -target-abi=n64 -O0 shows the bug:
(irrelevant assembly removed)
$BB0_1: # =>This Inner Loop Header: Depth=1 lw $1, 36($sp) # 4-byte Folded Reload ll $2, 0($1)
Which is sufficient to check that the change is correct.
lib/Target/Mips/MipsISelLowering.cpp | ||
---|---|---|
1506–1511 | Trailing whitespace on the lines with Mips::SLLV. |
Nit: For consistency please use 'IsABIN64'. It will be confusing to use '64' and 'N64' interchangably. Or preferably, refer to the pointer size instead to make it easier to keep track of the difference between integer and pointer sizes.