Page MenuHomePhabricator

[mips] Correct c.cond.fmt instruction definition.
ClosedPublic

Authored by sdardis on Sep 13 2016, 8:20 AM.

Details

Summary

Permit explicit $fcc<X> operand in c.cond.fmt instruction.

Add c.cond.fmt to the MIPS to microMIPS instruction mapping table.

Check that $fcc1 - $fcc7 are unusable for MIPS-I to MIPS-III for
c.cond.fmt, bc1t, bc1f.

Diff Detail

Repository
rL LLVM

Event Timeline

sdardis updated this revision to Diff 71176.Sep 13 2016, 8:20 AM
sdardis retitled this revision from to [mips] Correct c.cond.fmt instruction definition..
sdardis updated this object.
sdardis set the repository for this revision to rL LLVM.
sdardis added a subscriber: llvm-commits.
seanbruno accepted this revision.Sep 26 2016, 6:56 AM
seanbruno added a reviewer: seanbruno.
seanbruno added a subscriber: seanbruno.
This revision is now accepted and ready to land.Sep 26 2016, 6:57 AM
vkalintiris edited edge metadata.Sep 27 2016, 4:27 AM

Hi Sean, is your intention to LGTM this patch by accepting it, or to convey that it's working fine for *BSDs?

Hi Sean, is your intention to LGTM this patch by accepting it, or to convey that it's working fine for *BSDs?

I had hoped to express that it is an improvement to our FreeBSD toolchain. I cannot comment reliably on quality outside of style or the ability to be able to apply this review to a trunk checkout.

Hi Sean, is your intention to LGTM this patch by accepting it, or to convey that it's working fine for *BSDs?

I had hoped to express that it is an improvement to our FreeBSD toolchain. I cannot comment reliably on quality outside of style or the ability to be able to apply this review to a trunk checkout.

Ok, thanks! I'll take a look then.

vkalintiris requested changes to this revision.Sep 27 2016, 9:20 AM
vkalintiris edited edge metadata.

I'm getting a few test failures from the LLVM test-suite for MIPS32R2. Can you take a look at what's going wrong and update the patch before I review it? Given the symmetry of the code I'd expect, more or less, the same test failures to happen for microMIPS32R2 too but I'm not sure.

This revision now requires changes to proceed.Sep 27 2016, 9:20 AM
sdardis updated this revision to Diff 73449.Oct 4 2016, 4:01 AM
sdardis edited edge metadata.
sdardis removed rL LLVM as the repository for this revision.

Updated failing tests (xfailed disassembler test is now gone, illegal use of register is fixed)

seanbruno accepted this revision.Oct 4 2016, 6:28 AM
seanbruno edited edge metadata.

Applies cleanly and still seems to DTRT.

sdardis updated this revision to Diff 76400.Oct 31 2016, 7:37 AM
sdardis edited edge metadata.
sdardis set the repository for this revision to rL LLVM.

Rebase and ping.

seanbruno accepted this revision.Nov 22 2016, 7:41 AM
seanbruno edited edge metadata.

Looks good here at least form a user perspective.

sdardis updated this revision to Diff 79293.Nov 25 2016, 3:46 AM
sdardis edited edge metadata.
sdardis removed rL LLVM as the repository for this revision.

Rebase and ping.

zoran.jovanovic accepted this revision.Dec 30 2016, 8:35 AM
zoran.jovanovic edited edge metadata.

LGTM with a nit.

test/MC/Mips/mips1/invalid-mips2-wrong-error.s
17 ↗(On Diff #79293)

Nit: I can see that same error message is generated for bc1t and bc1f instructions and those are located in invalid-mips4.s test file.
Is expected error message different for these instructions or they should be placed in same file?
Same stands for corresponding test cases for mips2 and mips3.

sdardis updated this revision to Diff 83638.Jan 9 2017, 9:20 AM
sdardis edited edge metadata.
sdardis marked an inline comment as done.

Fix undiscovered bug in the handling of FCMP & friends. Internally generated c.cond.fmt were using the low bits of the register number to also select the fcc register.

Cleaned up error messages.

Addressed comments from reviewer.

@vkalintiris , could you take a look at this, you have to approve this as well.

vkalintiris accepted this revision.Jan 11 2017, 4:42 AM
vkalintiris edited edge metadata.

LGTM.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
4137–4138 ↗(On Diff #83638)

Parenthesizing the sub-expressions would make this a little bit easier to read.

lib/Target/Mips/MicroMipsInstrFPU.td
31 ↗(On Diff #83638)

s/thiese/these/, similarly below.

lib/Target/Mips/MipsInstrFPU.td
620 ↗(On Diff #83638)

s/thiese/these/, similarly below.

This revision is now accepted and ready to land.Jan 11 2017, 4:42 AM
This revision was automatically updated to reflect the committed changes.
sdardis marked 3 inline comments as done.

Thanks for the review.