[mips] wrong opcode for ll/sc instructions on mipsr6 when -integrated-as is used

[Jelena.Losic]

Summary

This commit resolves wrong opcode of instructions ll and sc for r6 architecutres, which was generated in method MipsTargetLowering::emitAtomicBinary.

[dsanders]

The code change looks correct but the test needs a couple changes.

test/Object/Mips/at.ll

1–2 ↗

(On Diff #36945)

This isn't an Object test, it's a CodeGen test and should be added to the appropriate test/CodeGen/Mips/*atomic*.ll.

The test can't distinguish the pre-R6 sc from the R6 sc. The patch needs to use FileCheck (http://llvm.org/docs/CommandGuide/FileCheck.html) instead of grep and should check the encoding to be sure the right instruction was used.

Also, please check for the correct ll, lld, and scd.

[Jelena.Losic]

Test implementation given in the patch is able to detect situation when pre-R6 opcode is generated for sc instruction.
Object code is generated by llc, disassembled by llvm-objdump (command line option -mattr=+mips64r6 is used) and llvm-objdump output is searched (by using grep - it is used in Object tests) for occurrence of sc mnemonic.
Because -mattr=+mips64r6 option is used llvm-objdump is not able to disassemble pre-R6 instruction opcodes and reports warning instead of instruction mnemonic.

llvm-objdump output - WITHOUT patch applied:
Disassembly of section .text:
atomic_load_test:

0:     f0 ff bd 67            daddiu  $sp, $sp, -16
4:     08 00 a3 df           ld            $3, 8($sp)
8:     00 00 04 24          addiu     $4, $zero, 0

llvm/llvm2/build/bin/llvm-objdump: warning: invalid instruction encoding <- without patch opcode is not recognized

10:    05 00 44 14          bne        $2, $4, 24
14:    00 00 00 00          nop
18:    00 00 05 24          addiu     $5, $zero, 0

llvm/llvm2/build/bin/llvm-objdump: warning: invalid instruction encoding <- without patch opcode is not recognized

20:    fa ff a0 10            beqz      $5, -20
24:    00 00 00 00          nop
28:    0f 00 00 00           sync
2c:    04 00 a2 af           sw          $2, 4($sp)
30:    09 00 e0 03          jr             $ra
34:    10 00 bd 67          daddiu  $sp, $sp, 16

llvm-objdump output - WITH patch applied:
Disassembly of section .text:
atomic_load_test:

 0:     f0 ff bd 67            daddiu  $sp, $sp, -16
 4:     08 00 a3 df           ld            $3, 8($sp)
 8:     00 00 04 24          addiu     $4, $zero, 0
 c:      36 00 62 7c           ll              $2, 0($3)
10:    05 00 44 14          bne        $2, $4, 24
14:    00 00 00 00          nop
18:    00 00 05 24          addiu     $5, $zero, 0
1c:    26 00 65 7c           sc            $5, 0($3)                   <- with patch opcode is recognized
20:    fa ff a0 10            beqz      $5, -20
24:    00 00 00 00          nop
28:    0f 00 00 00           sync
2c:    04 00 a2 af           sw          $2, 4($sp)
30:    09 00 e0 03          jr             $ra
34:    10 00 bd 67          daddiu  $sp, $sp, 16

Is it OK with you if I move test in CodeGen and modify it to use FileCheck instead of grep or some other approach is needed to check generated opcodes?
Having in mind that existing (non micromips) atomic tests do not check instruction opcodes is it OK to create new test in this case?

I will add checks for ll, lld and scd opcodes.

[dsanders]

In D13593#265806, @Jelena.Losic wrote:

Test implementation given in the patch is able to detect situation when pre-R6 opcode is generated for sc instruction.
Object code is generated by llc, disassembled by llvm-objdump (command line option -mattr=+mips64r6 is used) and llvm-objdump output is searched (by using grep - it is used in Object tests) for occurrence of sc mnemonic.
Because -mattr=+mips64r6 option is used llvm-objdump is not able to disassemble pre-R6 instruction opcodes and reports warning instead of instruction mnemonic.

That makes sense. So long as the tablegen-erated disassembler table has the encoding correct (and we test this elsewhere) then it can tell the difference. However, we can't rely on this in the long term since the invalid opcode will be re-used at some point and we don't know what the mnemonic will be.

Is it OK with you if I move test in CodeGen and modify it to use FileCheck instead of grep or some other approach is needed to check generated opcodes?

Moving the test to CodeGen and using FileCheck and a directive like 'CHECK: 26 00 65 7c sc $5, 0($3)' makes sense to me. If you update this review once you've made the change then I'll take a look at the new test.

It might be worth seeing if the -asm-show-inst to llc makes it possible to avoid assembling and disassembling in a CodeGen test. This option causes llc to emit the internal representation in a comment like so:

move $fp, $sp  # <MCInst #1480 OR
               #  <MCOperand Reg:8>
               #  <MCOperand Reg:20>
               #  <MCOperand Reg:21>>

In the case of 'sc', we should see 'SC' or 'SC_R6' depending on which was selected.

Having in mind that existing (non micromips) atomic tests do not check instruction opcodes is it OK to create new test in this case?

Generally speaking, we don't need to check opcodes in CodeGen tests. It should be sufficient to check the mnemonic. sc/scd/ll/lld are a bit trickier since we have multiple copies of these instructions and we need to make sure we selected the correct 'sc'. I'd prefer to use -asm-show-inst if possible but if we need to check opcodes to tell them apart then we'll have to do that.

I will add checks for ll, lld and scd opcodes.

Thanks

[Jelena.Losic]

Option -asm-show-inst enables writting test without disassembling as Daniel mentioned.

[dsanders]

Thanks. This patch is nearly there.

I will add checks for ll, lld and scd opcodes.

You haven't added these checks yet.

lib/Target/Mips/MipsISelLowering.cpp

1550 ↗

(On Diff #37599)

Nit: Remove unnecessary blank line

test/CodeGen/Mips/at1.ll

1 ↗

(On Diff #37599)

(about filename): Please rename at1.ll to something meaningful. There are some existing tests named atomic*.ll. Something similar would make sense to me.

[Jelena.Losic]

The test is changed, now there is checking for all instructions required, also test name is changed.

[dsanders]

Thanks. LGTM

llvm/trunk/lib/Target/Mips/MipsISelLowering.cpp

MachineBasicBlock * MipsTargetLowering::emitAtomicCmpSwap(MachineInstr *MI,
 
   MachineFunction *MF = BB->getParent();
   MachineRegisterInfo &RegInfo = MF->getRegInfo();
   const TargetRegisterClass *RC = getRegClassFor(MVT::getIntegerVT(Size * 8));
   const TargetInstrInfo *TII = Subtarget.getInstrInfo();
   DebugLoc DL = MI->getDebugLoc();
   unsigned LL, SC, ZERO, BNE, BEQ;
 
    if (Size == 4) {
-    LL = isMicroMips ? Mips::LL_MM : Mips::LL;
-    SC = isMicroMips ? Mips::SC_MM : Mips::SC;
+     if (isMicroMips) {
+       LL = Mips::LL_MM;
+       SC = Mips::SC_MM;
+     } else {
+       LL = Subtarget.hasMips32r6() ? Mips::LL_R6 : Mips::LL;
+       SC = Subtarget.hasMips32r6() ? Mips::SC_R6 : Mips::SC;
+     }
     ZERO = Mips::ZERO;
     BNE = Mips::BNE;
     BEQ = Mips::BEQ;
   } else {
-    LL = Mips::LLD;
-    SC = Mips::SCD;
+    LL = Subtarget.hasMips64r6() ? Mips::LLD_R6 : Mips::LLD;
+    SC = Subtarget.hasMips64r6() ? Mips::SCD_R6 : Mips::SCD;
     ZERO = Mips::ZERO_64;
     BNE = Mips::BNE64;
     BEQ = Mips::BEQ64;
   }
 
   unsigned Dest    = MI->getOperand(0).getReg();
   unsigned Ptr     = MI->getOperand(1).getReg();
   unsigned OldVal  = MI->getOperand(2).getReg();

llvm/trunk/test/CodeGen/Mips/atomicSCr6.ll

+; RUN: llc -asm-show-inst  -march=mips64el -mcpu=mips64r6 < %s -filetype=asm -o - | FileCheck %s -check-prefix=CHK64 
+; RUN: llc -asm-show-inst  -march=mipsel -mcpu=mips32r6 < %s -filetype=asm -o -| FileCheck %s -check-prefix=CHK32
+
+define internal i32 @atomic_load_test1() #0 {
+entry:
+
+  %load_add = alloca i32*, align 8
+  %.atomicdst = alloca i32, align 4
+  %0 = load i32*, i32** %load_add, align 8
+  %1 = load atomic i32, i32* %0 acquire, align 4
+  store i32 %1, i32* %.atomicdst, align 4
+  %2 = load i32, i32* %.atomicdst, align 4
+  
+  ret i32 %2
+}
+
+define internal i64 @atomic_load_test2() #0 {
+entry:
+
+  %load_add = alloca i64*, align 16
+  %.atomicdst = alloca i64, align 8
+  %0 = load i64*, i64** %load_add, align 16
+  %1 = load atomic i64, i64* %0 acquire, align 8
+  store i64 %1, i64* %.atomicdst, align 8
+  %2 = load i64, i64* %.atomicdst, align 8
+  
+  ret i64 %2
+}
+;CHK32:  LL_R6
+;CHK32:  SC_R6
+;CHK64:  LLD_R6
+;CHK64:  SCD_R6