This is an archive of the discontinued LLVM Phabricator instance.

[mips][microMIPS] Add CodeGen support for AND*, OR16, OR*, XOR*, NOT16 and NOR instructions
ClosedPublic

Authored by zbuljan on Jan 29 2016, 2:48 AM.

Details

Summary

The patch adds CodeGen support for microMIPSr6 AND16, ANDI16, AND, ANDI, OR16, OR, ORI, XOR16, XOR, XORI, NOT16 and NOR instructions:

  • updated .ll files with tests for microMIPSr6 AND*, OR16, OR*, XOR*, NOT16 and NOR instructions
  • added alias definitions for microMIPS and microMIPSr6 instructions
  • added DAG pattern definitions for proper selection of instructions
  • separated microMIPSr6 instructions from equivalent MIPS instructions using NotInMicroMips predicate
  • added tests for the standard encodings

Diff Detail

Repository
rL LLVM

Event Timeline

zbuljan updated this revision to Diff 46366.Jan 29 2016, 2:48 AM
zbuljan retitled this revision from to [mips][microMIPS] Add CodeGen support for AND*, OR16, OR*, XOR*, NOT16 and NOR instructions.
zbuljan updated this object.
zbuljan added subscribers: petarj, llvm-commits.
sdardis added a subscriber: sdardis.
sdardis added inline comments.
lib/Target/Mips/MicroMips32r6InstrInfo.td
1226 ↗(On Diff #46366)

Shouldn't the immediate by of type uimm16 ?

1229 ↗(On Diff #46366)

Here too.

lib/Target/Mips/MicroMipsInstrInfo.td
1006 ↗(On Diff #46366)

Can you provide a terminating } for the let Predicates = [InMicroMips] (line 966 of the diff} here.

1010 ↗(On Diff #46366)

Move the "let Predicates = [InMicroMips] in {" from line 1022 to line 1010. Then you can remove the closing branch on line 1020 and have this aliases section guarded clearly.

lib/Target/Mips/MipsInstrInfo.td
1368–1384 ↗(On Diff #46366)

These two "let AdditionalPredicates =" blocks can be joined together.

test/CodeGen/Mips/llvm-ir/and.ll
45 ↗(On Diff #46366)

Near ToT is not giving me this nop and the other nops. I see it with certain broken combinations of options (e.g. mips64 and +micromips).

155–252 ↗(On Diff #46366)

Check some other constants other than 4 that are encodable. Also add 1 or 2 cases where the value is not encodable in 16bit form and check that a normal andi is generated.

test/CodeGen/Mips/llvm-ir/or.ll
266 ↗(On Diff #46366)

As with the andi case, check some other short form encodable constants and add 1or 2 cases where the constant is not encodable in the 16bit form for andi.

test/MC/Mips/micromips32r6/valid.s
253 ↗(On Diff #46366)

Indentation for the "#encoding..."

test/MC/Mips/micromips64r6/valid.s
153–169 ↗(On Diff #46366)

Indentation for the "#encoding..."

sdardis requested changes to this revision.Apr 21 2016, 2:18 AM
sdardis edited edge metadata.
This revision now requires changes to proceed.Apr 21 2016, 2:18 AM
zbuljan updated this revision to Diff 57028.May 12 2016, 6:22 AM
zbuljan edited edge metadata.

Rebased to work with TOT.
Changed immediate to uimm16 in alias definitions for AND instruction.
Added DAG patterns to description clases of AND, OR and XOR instructions.
Added AddedComplexity parameter to definitions of AND16, OR16 and XOR16 instructions so they can be prioritized during instruction selection.
Updated and.ll and or.ll with IR code to check some constants that are and are not encodable in 16-bit form.
Added invalid tests for operand checking.

sdardis requested changes to this revision.May 16 2016, 5:19 AM
sdardis edited edge metadata.

Some nits and a question as to why llvm is generating 2 instructions instead of 1.

lib/Target/Mips/MicroMips32r6InstrInfo.td
605 ↗(On Diff #57028)

Requires AddedComplexity = 1.

1007 ↗(On Diff #57028)

Also requires AddedComplexity = 1

lib/Target/Mips/MicroMipsInstrInfo.td
1052 ↗(On Diff #57028)

This needs to be covered with "let Predicates = [InMicroMips] in {"

test/CodeGen/Mips/llvm-ir/or.ll
173–174 ↗(On Diff #57028)

Why is LLVM generating two instructions here rather than a single long form 'or'? Similarly for xor.

test/MC/Disassembler/Mips/micromips32r3/valid-el.txt
197 ↗(On Diff #57028)

This requires not16 and nor. The 32bit andi, ori, xori, not are not required as they already exist in this file.

test/MC/Disassembler/Mips/micromips32r3/valid.txt
197 ↗(On Diff #57028)

See my above comment.

test/MC/Disassembler/Mips/micromips32r6/valid.txt
315 ↗(On Diff #57028)

andi, ori, xori are already exist in this file, so there's no need to duplicate them.

test/MC/Mips/micromips-alu-instructions.s
132 ↗(On Diff #57028)

Already covered by and $9, $6, 17767

136 ↗(On Diff #57028)

Already covered by xor $9, $6, 17767

test/MC/Mips/micromips64r6/valid.s
15 ↗(On Diff #57028)

This also needs to check not16.

This revision now requires changes to proceed.May 16 2016, 5:19 AM
zbuljan added inline comments.May 17 2016, 4:06 AM
lib/Target/Mips/MicroMipsInstrInfo.td
1052 ↗(On Diff #57028)

It should be already covered with ISA_MICROMIPS predicate. Do you still want me to change it?

zbuljan updated this revision to Diff 59022.May 31 2016, 1:56 AM
zbuljan edited edge metadata.

Rebased to work with TOT.
Added AddedComplexity = 1 to ORI and NOT16 description classes.
Added DAG patterns for proper selection of ORI and XORI instructions.
Separated MIPS ANDI, ORI and XORI from equivalent microMIPS instructions using ISA_NOT_MICROMIPS predicate.
Updated not.ll with tests for NOR instruction.
Removed redundant asm/disasm tests.
Added asm/disasm tests for the instructions which were not covered.

Can you explain the shift away from "let AdditionalPredicates = [NotInMicroMips] in" to using ISA_NOT_MICROMIPS for ANDi, ORi, XORi?

test/CodeGen/Mips/llvm-ir/and.ll
28 ↗(On Diff #59022)

The check prefix MMR6 should be changed to MM or MMALL, since you're using it with microMIPSR3, microMIPSR6 and microMIPS64R6.

567–571 ↗(On Diff #59022)

These can be reduced down to 'ALL:'

test/MC/Mips/micromips/invalid.s
121 ↗(On Diff #59022)

This should also check 'not' with a register and immediate. This should also go into the other invalid files you're changing.

As I can see now there is no reason for usage of ISA_NOT_MICROMIPS instead of "let AdditionalPredicates = [NotInMicroMips] in" so I'll change it.

zbuljan updated this revision to Diff 60164.Jun 9 2016, 6:03 AM
zbuljan edited edge metadata.

Rebased to work with TOT.
Removed ISA_NOT_MICROMIPS because it is not needed and used "let AdditionalPredicates = [NotInMicroMips]" instead.
Replaced MMR6 check prefix with MM.
Added 'not' with a register and immediate to invalid tests.

sdardis accepted this revision.Jun 14 2016, 5:19 AM
sdardis edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Jun 14 2016, 5:19 AM
This revision was automatically updated to reflect the committed changes.