This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Jelena.Losic on Oct 9 2015, 7:20 AM.

Details

Summary

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

Diff Detail

Event Timeline

Jelena.Losic retitled this revision from to [mips] wrong opcode for ll/sc instructions on mipsr6 when -integrated-as is used.
Jelena.Losic updated this object.
Jelena.Losic added a reviewer: zoran.jovanovic.
Jelena.Losic added a subscriber: llvm-commits.
dsanders requested changes to this revision.Oct 12 2015, 5:54 AM
dsanders added a reviewer: dsanders.
dsanders added a subscriber: 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.

This revision now requires changes to proceed.Oct 12 2015, 5:54 AM

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.

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 edited edge metadata.

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

dsanders requested changes to this revision.Oct 27 2015, 10:39 AM
dsanders edited edge metadata.

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

Nit: Remove unnecessary blank line

test/CodeGen/Mips/at1.ll
1

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

This revision now requires changes to proceed.Oct 27 2015, 10:39 AM
Jelena.Losic edited edge metadata.

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

dsanders accepted this revision.Oct 28 2015, 5:32 PM
dsanders edited edge metadata.

Thanks. LGTM

This revision is now accepted and ready to land.Oct 28 2015, 5:32 PM
This revision was automatically updated to reflect the committed changes.