This is an archive of the discontinued LLVM Phabricator instance.

[mips] Fix aui/daui/dahi/dati for MIPSR6
ClosedPublic

Authored by sdardis on Jun 17 2016, 8:12 AM.

Details

Summary

For compatiblity with binutils, define these instructions to take
two registers with a 16bit unsigned immediate. Both of the registers
have to be same for dahi and dati.

Diff Detail

Event Timeline

sdardis updated this revision to Diff 61097.Jun 17 2016, 8:12 AM
sdardis retitled this revision from to [mips] Fix aui/daui/dahi/dati for MIPSR6.
sdardis updated this object.
sdardis added a reviewer: dsanders.
sdardis set the repository for this revision to rL LLVM.
sdardis added a subscriber: llvm-commits.
dsanders added inline comments.Jul 1 2016, 4:10 AM
lib/Target/Mips/MicroMips64r6InstrInfo.td
93–97

I think we need to split this out into a separate issue. I'm not very keen on the idea of working around a bug in the way tablegen handles tied operands, especially given that the workaround will prevent us from adding CodeGen support.

As you mention in the comment, the issue is that CVT_Tied is implemented such that it copies the MCOperand that this operand is tied to without checking if the current operand would render to the same thing. As a result, the matcher silently ignores bad input and emits a valid instruction that isn't what the user specified. It should error out instead.

I'll see if I can find a way to fix tablegen.

dsanders edited edge metadata.Jul 5 2016, 7:44 AM

I've posted a possible solution to the constraint problem as D21994.

dsanders requested changes to this revision.Jul 28 2016, 7:57 AM
dsanders edited edge metadata.

This will need reworking to account for D21994 having been committed. I'll review this once it's been rebased.

This revision now requires changes to proceed.Jul 28 2016, 7:57 AM
sdardis updated this revision to Diff 66118.Jul 29 2016, 7:04 AM
sdardis edited edge metadata.
sdardis removed rL LLVM as the repository for this revision.

Rebased on account of D21994 being committed.

I have a couple minor comments, the main one is the dubious disassembler test.

lib/Target/Mips/Disassembler/MipsDisassembler.cpp
601–604

GPR32 -> GPR64

lib/Target/Mips/Mips32r6InstrInfo.td
319

It feels wrong to have a simm16 immediate in the instruction but only accept uimm16 inputs in the assembly but I agree that this is the intended behaviour.

I think we should render it to the instruction as a simm16 so that the MCInst's are consistent though.

test/MC/Disassembler/Mips/micromips64r6/valid.txt
38–40

This doesn't look correct to me. I haven't double-checked the encodings but there's only one bit different between the two instructions yet two things are different in the assembly (dahi->dati, and $16 -> $17)

Also, could you comment on why it isn't $3 any more?

sdardis updated this revision to Diff 68842.Aug 22 2016, 3:43 AM
sdardis edited edge metadata.

Addressed review comments.

sdardis added inline comments.Aug 24 2016, 6:40 AM
test/MC/Disassembler/Mips/micromips64r6/valid.txt
38–40

The change was due to a bug in the decoder, it was reading the minor opcode as the register for microMIPS64r6. MIPSR6 has the fields swapped in comparison.

Zoran, would you be able to take a look at this? Thanks.

zoran.jovanovic accepted this revision.Aug 25 2016, 6:55 AM
zoran.jovanovic edited edge metadata.

LGTM.

sdardis updated this object.Sep 16 2016, 6:59 AM
sdardis edited edge metadata.

Daniel, can you accept this revision?

dsanders accepted this revision.Oct 13 2016, 3:18 AM
dsanders edited edge metadata.

Daniel, can you accept this revision?

Sorry for the delay, I thought I'd redirected this temporary email address to my new one but apparently not

This revision is now accepted and ready to land.Oct 13 2016, 3:18 AM
sdardis closed this revision.Oct 14 2016, 3:12 AM

No problem. Committed as r284218.