[PATCH] D41439: [mips] Properly select abs and sqrt instructions
Simon Dardis via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 9 01:50:57 PST 2018
sdardis added a comment.
This looks
================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:1627
defm D_MMR6 : CMP_CC_MMR6<0b010101, "d", FGR64Opnd, II_CMP_CC_D>;
-def ABS_S_MMR6 : StdMMR6Rel, ABS_S_MMR6_ENC, ABS_S_MMR6_DESC, ISA_MICROMIPS32R6;
-def ABS_D_MMR6 : StdMMR6Rel, ABS_D_MMR6_ENC, ABS_D_MMR6_DESC, ISA_MICROMIPS32R6;
+let AddedComplexity=1 in {
+ def ABS_S_MMR6 : StdMMR6Rel, ABS_S_MMR6_ENC, ABS_S_MMR6_DESC, ISA_MICROMIPS32R6;
----------------
Spaces around the '='.
================
Comment at: lib/Target/Mips/MicroMips32r6InstrInfo.td:1655
ISA_MICROMIPS32R6;
-def SQRT_S_MMR6 : StdMMR6Rel, SQRT_S_MMR6_ENC, SQRT_S_MMR6_DESC,
- ISA_MICROMIPS32R6;
-def SQRT_D_MMR6 : StdMMR6Rel, SQRT_D_MMR6_ENC, SQRT_D_MMR6_DESC,
- ISA_MICROMIPS32R6;
+let AddedComplexity=1 in {
+ def SQRT_S_MMR6 : StdMMR6Rel, SQRT_S_MMR6_ENC, SQRT_S_MMR6_DESC,
----------------
Here too.
================
Comment at: lib/Target/Mips/MicroMipsInstrFPU.td:96
+multiclass ABSS_MMM<string opstr, InstrItinClass Itin,
+ SDPatternOperator OpNode= null_frag> {
+ def _D32_MM : MMRel, ABSS_FT<opstr, AFGR64Opnd, AFGR64Opnd, Itin, OpNode>,
----------------
Nit, align the "SDPatternOperator [...]" to under the 'string' in the line above, spaces around the '='.
================
Comment at: lib/Target/Mips/MicroMipsInstrFPU.td:101
+ }
+ def _D64_MM : ABSS_FT<opstr, FGR64Opnd, FGR64Opnd, Itin, OpNode>,
+ ISA_MICROMIPS, FGR_64 {
----------------
FIXME: This needs to be part of the instruction mapping tables.
================
Comment at: lib/Target/Mips/MicroMipsInstrFPU.td:197
+ string DecoderNamespace = "MicroMips";
+ }
def MTHC1_MM : MMRel, MTC1_64_FT<"mthc1", AFGR64Opnd, GPR32Opnd, II_MTHC1>,
----------------
Nit: the closing '}' should be aligned with the def or class it's apart of.
================
Comment at: lib/Target/Mips/MipsInstrFPU.td:462-464
def FSQRT_S : MMRel, StdMMR6Rel, ABSS_FT<"sqrt.s", FGR32Opnd, FGR32Opnd,
II_SQRT_S, fsqrt>, ABSS_FM<0x4, 16>, ISA_MIPS2;
defm FSQRT : ABSS_M<"sqrt.d", II_SQRT_D, fsqrt>, ABSS_FM<0x4, 17>, ISA_MIPS2;
----------------
Nit, indent this by 2 columns.
================
Comment at: test/CodeGen/Mips/abs_sqrt.ll:1
+; RUN: llc -march=mips -mcpu=mips32 -asm-show-inst < %s | FileCheck %s --check-prefix=MIPS32
+; RUN: llc -march=mips -mcpu=mips32 -mattr=+fp64 -asm-show-inst < %s | FileCheck %s --check-prefix=MIPS32FP64
----------------
Modify the existing file test/CodeGen/Mips/llvm-ir/sqrt.ll for the square root tests.
Add test/CodeGen/Mips/llvm-ir/abs.ll for the abs tests.
================
Comment at: test/CodeGen/Mips/abs_sqrt.ll:2
+; RUN: llc -march=mips -mcpu=mips32 -asm-show-inst < %s | FileCheck %s --check-prefix=MIPS32
+; RUN: llc -march=mips -mcpu=mips32 -mattr=+fp64 -asm-show-inst < %s | FileCheck %s --check-prefix=MIPS32FP64
+; RUN: llc -march=mips -mcpu=mips32 -mattr=+micromips -asm-show-inst < %s | FileCheck %s --check-prefix=MM
----------------
cpu should be mips32r2.
================
Comment at: test/CodeGen/Mips/abs_sqrt.ll:3-4
+; RUN: llc -march=mips -mcpu=mips32 -mattr=+fp64 -asm-show-inst < %s | FileCheck %s --check-prefix=MIPS32FP64
+; RUN: llc -march=mips -mcpu=mips32 -mattr=+micromips -asm-show-inst < %s | FileCheck %s --check-prefix=MM
+; RUN: llc -march=mips -mcpu=mips32 -mattr=+micromips,+fp64 -asm-show-inst < %s | FileCheck %s --check-prefix=MMFP64
+; RUN: llc -march=mips -mcpu=mips32r6 -mattr=+micromips -asm-show-inst < %s | FileCheck %s --check-prefix=MMR6
----------------
cpu should be mips32r3.
================
Comment at: test/MC/Disassembler/Mips/micromips32/abs_sqrt.txt:1
+# RUN: llvm-mc --disassemble -arch=mips -mcpu=mips32 -mattr=+micromips < %s | FileCheck %s --check-prefix=MM
+
----------------
This should go in test/MC/Disassembler/Mips/micromips32r3/valid.txt and the little endian disassembly in test/MC/Disassembler/Mips/micromips32r3/valid-el.txt
================
Comment at: test/MC/Disassembler/Mips/micromips32/abs_sqrt_fp64.txt:1
+# RUN: llvm-mc --disassemble -arch=mips -mcpu=mips32 -mattr=+micromips,+fp64 < %s | FileCheck %s --check-prefix=MMFP64
+
----------------
This should go in test/MC/Disassembler/Mips/micromips32r3/valid-fp64.txt and provide little endian disassembly in test/MC/Disassembler/Mips/micromips32r3/valid-fp64-el.txt.
The -mcpu= should be mips32r3. You should drop the explicit check prefix and use CHECK: for the check lines instead. Also keep the hex and CHECK on the same line, i.e.
0x54 0x0c 0x0a 0x3b
# MMFP64: sqrt.s $f0, $f12
Should be:
0x54 0x0c 0x0a 0x3b # CHECK: sqrt.s $f0, $f12
================
Comment at: test/MC/Disassembler/Mips/mips32/abs_sqrt.txt:1
+# RUN: llvm-mc --disassemble -arch=mips -mcpu=mips32 < %s | FileCheck %s --check-prefix=MIPS32
+
----------------
This file can be entirely dropped, existing disassembly tests cover it.
================
Comment at: test/MC/Disassembler/Mips/mips32/abs_sqrt_fp64.txt:1
+# RUN: llvm-mc --disassemble -arch=mips -mcpu=mips32 -mattr=+fp64 < %s | FileCheck %s --check-prefix=MIPS32FP64
+
----------------
Rename this file to valid-fp64.txt and place copies in test/MC/Disassembler/Mips/mips32r{2, 3, 5}/
Also provide little endian disassembly files. You can drop the explicit check prefix and just use 'CHECK'. See our existing disassembly files and copy their style.
================
Comment at: test/MC/Disassembler/Mips/mips32r6/abs_sqrt.txt:1
+# RUN: llvm-mc --disassemble -arch=mips -mcpu=mips32r6 -mattr=+micromips < %s | FileCheck %s --check-prefix=MMR6
+
----------------
This file can be dropped as the existing tests cover it.
================
Comment at: test/MC/Mips/abs_sqrt.s:1
+# RUN: llvm-mc -arch=mips -mcpu=mips32 -show-encoding < %s | FileCheck %s --check-prefix=MIPS32
+# RUN: llvm-mc -arch=mips -mcpu=mips32 -mattr=+fp64 -show-encoding < %s | FileCheck %s --check-prefix=MIPS32FP64
----------------
Rather than providing this as a new test, you can modify existing tests to cover almost all the variants here.
For the mips32 case, modify or ensure that test/MC/Mips/mips{1, 2, 32, 32r2, 32r3, 32r5}/valid.s produce the non-micromips AFGR64 instruction. If you look at the 'abs.d' case in test/MC/Mips/micromips/valid.s, you'll see how it should look.
I've highlighted the additional changes for this file.
================
Comment at: test/MC/Mips/abs_sqrt.s:2
+# RUN: llvm-mc -arch=mips -mcpu=mips32 -show-encoding < %s | FileCheck %s --check-prefix=MIPS32
+# RUN: llvm-mc -arch=mips -mcpu=mips32 -mattr=+fp64 -show-encoding < %s | FileCheck %s --check-prefix=MIPS32FP64
+# RUN: llvm-mc -arch=mips -mcpu=mips32 -mattr=+micromips -show-encoding < %s | FileCheck %s --check-prefix=MM
----------------
Provide this variant in test/MC/Mips/mips32r{2,3,5}/valid-fp64.s. The cpu should correspond to the directory it's in. Also test which instruction the assembly has determined it is with -show-inst.
================
Comment at: test/MC/Mips/abs_sqrt.s:3
+# RUN: llvm-mc -arch=mips -mcpu=mips32 -mattr=+fp64 -show-encoding < %s | FileCheck %s --check-prefix=MIPS32FP64
+# RUN: llvm-mc -arch=mips -mcpu=mips32 -mattr=+micromips -show-encoding < %s | FileCheck %s --check-prefix=MM
+# RUN: llvm-mc -arch=mips -mcpu=mips32 -mattr=+micromips,+fp64 -show-encoding < %s | FileCheck %s --check-prefix=MMFP64
----------------
Check which instructions are produced from test/MC/Mips/micromips/valid.s with the -show-inst option.
================
Comment at: test/MC/Mips/abs_sqrt.s:4
+# RUN: llvm-mc -arch=mips -mcpu=mips32 -mattr=+micromips -show-encoding < %s | FileCheck %s --check-prefix=MM
+# RUN: llvm-mc -arch=mips -mcpu=mips32 -mattr=+micromips,+fp64 -show-encoding < %s | FileCheck %s --check-prefix=MMFP64
+# RUN: llvm-mc -arch=mips -mcpu=mips32r6 -mattr=+micromips -show-encoding < %s | FileCheck %s --check-prefix=MMR6
----------------
Provide this variant in test/MC/Mips/micromips/valid-fp64.s. Also the cpu should be mips32r3.
================
Comment at: test/MC/Mips/abs_sqrt.s:5
+# RUN: llvm-mc -arch=mips -mcpu=mips32 -mattr=+micromips,+fp64 -show-encoding < %s | FileCheck %s --check-prefix=MMFP64
+# RUN: llvm-mc -arch=mips -mcpu=mips32r6 -mattr=+micromips -show-encoding < %s | FileCheck %s --check-prefix=MMR6
+
----------------
Check which instructions are produced from test/MC/Mips/micromips32r6/valid.s with the -show-inst option.
================
Comment at: test/MC/Mips/abs_sqrt.s:28
+# MIPS32FP64: abs.d $f0, $f12 # encoding: [0x46,0x20,0x60,0x05]
+# MM: abs.d $f0, $f12 # encoding: [0x54,0x0c,0x23,0x7b]
+# MMFP64: abs.d $f0, $f12 # encoding: [0x54,0x0c,0x23,0x7b]
----------------
This case is already covered as you've modified test/MC/Mips/micromips/valid.s
https://reviews.llvm.org/D41439
More information about the llvm-commits
mailing list