[PATCH] D34507: [mips] Generate NMADD and NMSUB instructions when fneg node is present

Simon Dardis via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sat Jun 24 08:25:08 PDT 2017


sdardis requested changes to this revision.
sdardis added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: lib/Target/Mips/MipsInstrFPU.td:845
 
+//To generate NMADD and NMSUB instructions when fneg node is present
+let AdditionalPredicates = [NoNaNsFPMath, HasMadd4] in {
----------------
Nit: Space after the //.


================
Comment at: lib/Target/Mips/MipsInstrFPU.td:847-863
+  def : MipsPat<(fneg (fsub (fmul FGR32Opnd:$fs, FGR32Opnd:$ft), FGR32Opnd:$fr)),
+                (NMSUB_S FGR32Opnd:$fr, FGR32Opnd:$fs, FGR32Opnd:$ft)>, INSN_MIPS4_32R2_NOT_32R6_64R6;
+
+  def : MipsPat<(fneg (fsub (fmul AFGR64Opnd:$fs, AFGR64Opnd:$ft), AFGR64Opnd:$fr)),
+                (NMSUB_D32 AFGR64Opnd:$fr, AFGR64Opnd:$fs, AFGR64Opnd:$ft)>, FGR_32, INSN_MIPS4_32R2_NOT_32R6_64R6;
+
+  def : MipsPat<(fneg (fadd (fmul FGR32Opnd:$fs, FGR32Opnd:$ft), FGR32Opnd:$fr)),
----------------
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. 


================
Comment at: test/CodeGen/Mips/nmadd.ll:1-2
+; RUN: llc < %s -march=mips64el -mcpu=mips64   -target-abi=n64 -enable-no-nans-fp-math | FileCheck %s -check-prefixes=ALL,64-NM-OP
+; RUN: llc < %s -march=mips64el -mcpu=mips64r2 -target-abi=n64 -enable-no-nans-fp-math | FileCheck %s -check-prefixes=ALL,64R2-NM-OPERATION
+
----------------
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.


================
Comment at: test/CodeGen/Mips/nmadd.ll:1-2
+; RUN: llc < %s -march=mips64el -mcpu=mips64   -target-abi=n64 -enable-no-nans-fp-math | FileCheck %s -check-prefixes=ALL,64-NM-OP
+; RUN: llc < %s -march=mips64el -mcpu=mips64r2 -target-abi=n64 -enable-no-nans-fp-math | FileCheck %s -check-prefixes=ALL,64R2-NM-OPERATION
+
----------------
sdardis wrote:
> 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.
You can reuse the same check prefix, also consider sorting to just CHECK-NM.


================
Comment at: test/CodeGen/Mips/nmadd.ll:4
+
+
+; Function Attrs: norecurse nounwind readnone
----------------
This test also needs a comment describing what it's testing.


================
Comment at: test/CodeGen/Mips/nmadd.ll:5-6
+
+; Function Attrs: norecurse nounwind readnone
+define float @add1(float %f, float %g, float %h) local_unnamed_addr #0 {
+entry:
----------------
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.


================
Comment at: test/CodeGen/Mips/nmadd.ll:8
+entry:
+;ALL-LABEL: @add1
+
----------------
Space after the ';', drop the @ and a colon after the name to match the label properly.


https://reviews.llvm.org/D34507





More information about the llvm-commits mailing list