This is an archive of the discontinued LLVM Phabricator instance.

[mips] Clang ll/sc illegal instruction on mips64r2 with -O0
AbandonedPublic

Authored by petarj on Oct 12 2015, 6:49 AM.

Details

Summary

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.

Diff Detail

Event Timeline

Jelena.Losic retitled this revision from to [mips] Clang ll/sc illegal instruction on mips64r2 with -O0.
Jelena.Losic updated this object.
Jelena.Losic added a reviewer: zoran.jovanovic.
Jelena.Losic added a subscriber: llvm-commits.
vkalintiris requested changes to this revision.Oct 12 2015, 7:28 AM
vkalintiris added a reviewer: vkalintiris.
vkalintiris added a subscriber: vkalintiris.

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 ↗(On Diff #37103)

You use %s twice here. We should pick only one method for passing the code to llc.

This revision now requires changes to proceed.Oct 12 2015, 7:28 AM
zoran.jovanovic edited edge metadata.
This comment was removed by Jelena.Losic.
Jelena.Losic edited edge metadata.

I put code changes according to Vasileios suggestions.

Jelena.Losic edited edge metadata.
vkalintiris added inline comments.Oct 20 2015, 4:53 AM
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
2–20

The test seems fine but could use a more meaningful filename, eg. test-atomic-cmp-swap.ll or something similar?

petarj added a subscriber: petarj.Oct 20 2015, 8:01 AM

In the method emitAtomicCmpPartword, when condition Subtaget.isGP64() is satisfied, there are included 64bit versions of atithmetic instructions.

dsanders requested changes to this revision.Oct 28 2015, 5:53 PM
dsanders added a reviewer: dsanders.

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
1410–1413

Variables should start with a capital letter.

1411–1414

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.

1479

You can use ABI.GetZeroReg() to pick the right zero register

Just for completeness, pointers can use ABI.GetNullPtr()

1488

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.

1490

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.

This revision now requires changes to proceed.Oct 28 2015, 5:53 PM
Jelena.Losic edited edge metadata.

Arithmetic instructions that are used are selected according to ABI's integer/pointer width.

dsanders requested changes to this revision.Nov 4 2015, 6:00 AM
dsanders edited edge metadata.

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
1410–1411

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.

1420–1436

Some of these should pick i64 for 64-bit integers (N32/N64), others should pick i64 for 64-bit pointers (N64 only).

1477–1478

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.

1483

SLL/DSLL depending on pointer size

1488

SLL/DSLL depending on pointer size

1491–1492

IsABI64 and GetZeroReg() do not agree with eachother.

1492

SLL/DSLLV based on pointer size (I think)

1495–1496

IsABI64 and GetZeroReg() do not agree with eachother.

1497

SLLV/DSLLV. It's not immediately obvious whether this is integer or pointer

1501

SLLV/DSLLV. It's not immediately obvious whether this is integer or pointer

This revision now requires changes to proceed.Nov 4 2015, 6:00 AM
Jelena.Losic edited edge metadata.

I tried to distinguish width of integer/pointer used due to ABI.

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).

dsanders requested changes to this revision.Nov 23 2015, 9:36 AM
dsanders edited edge metadata.
dsanders added inline comments.
lib/Target/Mips/MipsISelLowering.cpp
1409–1410

Use ABI.ArePtrs64bit() and ABI.AreGprs64bit() instead.

1488–1491

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);
1488–1492

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?

This revision now requires changes to proceed.Nov 23 2015, 9:36 AM

AFAIU, the original problem has been fixed with http://reviews.llvm.org/D14397 , so we can abandon this change and close the issue.

sdardis added a subscriber: sdardis.Apr 7 2016, 3:55 AM

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
1505–1510

Trailing whitespace on the lines with Mips::SLLV.

Jelena, Can you mark this revision as abandoned, as D18995 fixes the issue? Thanks.

petarj commandeered this revision.Apr 14 2016, 6:46 AM
petarj abandoned this revision.
petarj added a reviewer: Jelena.Losic.

Abandoning the issue as the other change [1] fixed it.

[1] http://reviews.llvm.org/D18995 .