This is an archive of the discontinued LLVM Phabricator instance.

[mips] Generate NMADD and NMSUB instructions when fneg node is present
ClosedPublic

Authored by stanislav.ocovaj on Jun 22 2017, 5:03 AM.

Details

Summary

This patch enables generation of NMADD and NMSUB instructions when fneg node is present. These instructions are currently only generated if fsub node is present.

Diff Detail

Event Timeline

sdardis requested changes to this revision.Jun 24 2017, 8:25 AM

Please upload patches with full context next time. The arc tool generates the correct context, or use git diff -U9999. Also, the line endings for .td are windows/dos endings, they should be unix line endings.

Two major changes, I've briefly outline them in line. The optimization pattern should be like the multiclass BrcondPats in MipsInstrInfo.td, so that it can be instantiated in MicroMipsInstrInfo.td as well.

The second is an increase in test coverage. The nmadd & nmsub instructions were added to MIPS-IV, so they are present in mips4, mips64{r2, r3, r5}, mips32r{2, 3, 5} and micromips. So they need to be tested at least for mips4, mips64, misp64r2, mips32r2 and micromips. We need negative testing for mips3, mips32r1 and (micromips)mips{32,64}r6.

Can you also test the output of -asm-show-inst as well. We need to ensure that _MM versions don't occur for standard mips encodings and _MM versions only occur for micromips.

lib/Target/Mips/MipsInstrFPU.td
845

Nit: Space after the //.

847–863

This should be written as multiclass, taking the instruction and register operand as parameters. You can then instantiate it three times for the different instructions.

This also requires NotInMicroMips in the AdditionalPredicates. You'll need to duplicate this in the micromips fpu td file with the appropriate micromips instructions and InMicroMips for the AdditionalPredicate.

test/CodeGen/Mips/nmadd.ll
2–3

Test that this works also for micromips, mips4, mips32r2 with and without -mattr=+fp64.

Also, test that n{madd/msub} does not appear for mips3, mips32 and mips32r6 + micromips32r6.

2–3

You can reuse the same check prefix, also consider sorting to just CHECK-NM.

5

This test also needs a comment describing what it's testing.

6–7

Drop the "; Function Attrs: norecurse nounwind readnone" and " local_unnamed_addr #0 ". they're just visual clutter / metadata that provides no purpose for this test.

9

Space after the ';', drop the @ and a colon after the name to match the label properly.

This revision now requires changes to proceed.Jun 24 2017, 8:25 AM
stanislav.ocovaj edited edge metadata.
sdardis requested changes to this revision.Aug 11 2017, 7:52 AM

You should also add a test with -asm-show-inst for micromips and show that we only generate instructions with _MM suffixes.

Some more comments inlined.

lib/Target/Mips/MicroMipsInstrFPU.td
274

These predicates don't match the ones for standard mips.

lib/Target/Mips/MipsInstrFPU.td
863

Spurious blank line.

test/CodeGen/Mips/nmadd.ll
10

That should be -mattr=micromips. You can skip testing for micromips64r6 as that backend is going to be removed.

This revision now requires changes to proceed.Aug 11 2017, 7:52 AM
stanislav.ocovaj edited edge metadata.

The combination -mcpu=mips32r6 -mattr=micromips cannot be tested at the moment due to the following error: "LLVM ERROR: ran out of registers during register allocation". This issue has been reported.

LGTM with the inline nits addressed.

For microMIPS, we need to change the predicates guarding various instructions.

lib/Target/Mips/MicroMipsInstrFPU.td
273

This also requires NotMips32r6, as the nmadd, nmsub are missing their relevant predicates.

test/CodeGen/Mips/nmadd.ll
11

The check prefixes here are incorrect, they should match the mips32r6 ones.

stanislav.ocovaj marked 2 inline comments as done.Aug 24 2017, 9:11 AM
This revision was automatically updated to reflect the committed changes.