This is an archive of the discontinued LLVM Phabricator instance.

Add fcc registers to c.cond.fmt instruction definition.
Needs ReviewPublic

Authored by vmedic on Aug 4 2014, 6:40 AM.

Details

Summary

Current implementation of c.cond.fmt instructions only accept default cc0 register. This patch enables the instruction to accept other fcc registers. The aliases with default fcc0 registers are also defined.

Diff Detail

Event Timeline

vmedic updated this revision to Diff 12157.Aug 4 2014, 6:40 AM
vmedic retitled this revision from to Add fcc registers to c.cond.fmt instruction definition..
vmedic updated this object.
vmedic edited the test plan for this revision. (Show Details)
vmedic added reviewers: dsanders, zoran.jovanovic.
vmedic changed the visibility from "Public (No Login Required)" to "Custom Policy".
vmedic changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 6 2014, 3:05 AM
dsanders added inline comments.Aug 11 2014, 2:42 AM
lib/Target/Mips/MipsInstrFPU.td
235

Given that CC is always FCCRegsOpnd do we really need it to be an argument? I think we could just use it directly.

264

You need to update ISA_MIPS* adverbs. Support for multiple $fcc registers was added in MIPS-IV and MIPS32. Prior to that only the form without $fcc registers was permitted.

273

Nit: space after comma

vmedic added inline comments.Aug 11 2014, 3:13 AM
lib/Target/Mips/MipsInstrFPU.td
235

This was introduced to enable alias definitions that use FCC0 as a default value.

264

That would mean that we need two separated definitions for c.cond.fmt instructions, one for Mips-IV and Mips32 that allows $fcc registers and one for prior architectures that doesn't. Is that right?

dsanders added inline comments.Aug 11 2014, 3:59 AM
lib/Target/Mips/MipsInstrFPU.td
235

I mean the argument for the class not the operand. Your definition will look like this:

class C_COND_FT<string CondStr, string Typestr, RegisterOperand RC, InstrItinClass itin>  :
   InstSE<(outs), (ins FCCRegOpnd:$cc, RC:$fs, RC:$ft),
          !strconcat("c.", CondStr, ".", Typestr, "\t $cc, $fs, $ft"),
          [], itin, FrmFR>;
264

You should only need the definitions you have now but with corrected adverbs. It's possible to have an InstAlias enabled even when the Instruction it aliases is disabled.

vmedic updated this revision to Diff 12447.Aug 13 2014, 7:20 AM

Changed instruction definition to use FCCRegsOpnd in base class. Changed predicates for instruction and aliases.

dsanders accepted this revision.Aug 13 2014, 12:52 PM
dsanders edited edge metadata.

LGTM with a whitespace nit. Could you align the start of the operands to match the neighbouring instructions in each of the test files?

It's not necessary for this commit but we ought to start checking that the encodings are correct soon. Most instructions only test that the parser accepts the syntax at the moment

This revision is now accepted and ready to land.Aug 13 2014, 12:52 PM
vmedic updated this revision to Diff 14445.Oct 6 2014, 4:33 AM
vmedic added a reviewer: sstankovic.

This patch fixes the failing tests that caused the initial patch to be reverted. Instruction formats for assembler and code gen versions of conditional instructions have been separated, so at the moment fcc0-fcc7 registers are allowed for assembler while on code gen it will be still fixed to fcc0.

vmedic planned changes to this revision.Jan 26 2015, 9:20 AM

Revision reverted, changes required.

vmedic updated this revision to Diff 18893.Jan 28 2015, 8:39 AM
vmedic added a subscriber: Unknown Object (MLST).

A bug in the previous version of the patch has been fixed. Disassembler tests are added, and corresponding cases from xfailed tests removed.

This revision is now accepted and ready to land.Jan 28 2015, 8:39 AM
vmedic requested a review of this revision.Feb 4 2015, 6:08 AM
vmedic edited edge metadata.
dsanders resigned from this revision.Jul 18 2019, 7:01 PM
test/MC/Disassembler/Mips/mips32/valid-mips32.txt