This commit resolves wrong opcode of instructions ll and sc for r6 architecutres, which was generated in method MipsTargetLowering::emitAtomicBinary.
Details
Diff Detail
Event Timeline
The code change looks correct but the test needs a couple changes.
test/Object/Mips/at.ll | ||
---|---|---|
1–2 | 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. |
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.
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
Option -asm-show-inst enables writting test without disassembling as Daniel mentioned.
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 | ||
---|---|---|
1549 | 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. |
The test is changed, now there is checking for all instructions required, also test name is changed.
Nit: Remove unnecessary blank line