The type of Imm in MipsDisassembler.cpp was incorrect since SignExtend64 return int64_t type.As per the MIPSr6 doc ,the offset is added to the address of the instruction following the branch (not the branch itself), to form a PC-relative effective target address hence “4” is added to the offset. The offset of some test case are update to reflect the changes due to “ + 4 ” offset and new test case for negative offset are added.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Can you update the following tests too?
LLVM :: ExecutionEngine/RuntimeDyld/Mips/ELF_N64R6_relocations.s LLVM :: ExecutionEngine/RuntimeDyld/Mips/ELF_O32R6_relocations.s LLVM :: MC/Disassembler/Mips/mips64r6/valid-mips64r6.txt (You forgot to update the BALC instruction).
They are failing on my machine with check-all.
Can you check whether the assemble/disassemble/assemble or disassemble/assemble/disassemble round-trip works for the affected instructions?
Assuming it does: LGTM with the FIXME's removed.
test/MC/Disassembler/Mips/mips32r6/valid-mips32r6.txt | ||
---|---|---|
36 ↗ | (On Diff #49609) | Are these FIXME's still true? If you assemble the mnemonics from the disassembler do you get the same opcodes? |
test/MC/Disassembler/Mips/mips32r6/valid-mips32r6.txt | ||
---|---|---|
36 ↗ | (On Diff #49609) | The encoding and decoding are not inverse of each other. Since while decoding the offset is left shifted by 2 and then 4 is added but in case of encoding it will just right shift by 2. Thus for example encoding 0x18 0x42 0x01 0x4d -->disassemble -->bgezalc $2, 1336 So these FIXME's will always be true |
test/MC/Disassembler/Mips/mips32r6/valid-mips32r6.txt | ||
---|---|---|
36 ↗ | (On Diff #49609) | That's definitely a bug, they should be inverses of each other. I've just looked at the encode function and I think it's wrong for the path this test will use. The immediate path is just 'X >> 2' but the symbol reference path is '(X - 4) >> 2'. Would you be willing to fix the immediate-encode path as part of this patch? That way we can drop these FIXME's. |
test/MC/Disassembler/Mips/mips32r6/valid-mips32r6.txt | ||
---|---|---|
36 ↗ | (On Diff #49609) | It appears I'm mistaken on this but we think the current behaviour is weird/wrong in both GAS and IAS. In GAS, immediates are absolute addresses that are modified with +4 for unknown reasons. In IAS, immediates are currently offsets and are not modified. Your change matches GAS's behaviour so: LGTM with the 'FIXME: ' comments changed to '# The encode/decode functions are not inverses of each other in the immediate case' |
Hi Nitesh, you can go ahead and commit this. But you should change the comments from:
# The encode/decode functions are not inverses of each other.
to:
# The encode/decode functions are not inverses of each other in the immediate case
first, ie. you missed the in the immediate case bit :)