[PATCH] D13235: [mips][microMIPS] Fix an issue with selecting sqrt instruction

[Daniel Sanders via llvm-commits]

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

> [mips][microMIPS] Fix an issue with selecting sqrt instruction

>  This patch fixes bug 2206 (https://dmz-portal.mips.com/bugz/show_bug.cgi?id=2206).


Could you rephrase the subject and summary to explain the problem rather than linking to the MIPS internal bugzilla? The LLVM commit messages should stand on their own and shouldn't depend on external sources to understand the problem/solution in case that information becomes inaccessible for some reason. In this case the linked ticket doesn't explain the problem either, it just says 'Several micromips builders started to fail'. I had to dig through several links to get to the buildbot log in order to figure out what kind of problem it was.


================
Comment at: lib/Target/Mips/MipsInstrFPU.td:356-357
@@ -355,7 +355,4 @@
 
-let AdditionalPredicates = [NotInMicroMips] in {
-def FSQRT_S : MMRel, ABSS_FT<"sqrt.s", FGR32Opnd, FGR32Opnd, II_SQRT_S, fsqrt>,
-              ABSS_FM<0x4, 16>, ISA_MIPS2;
-}
-
+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;
----------------
I notice you've added StdMMR6Rel but your test doesn't try microMIPS32R6. Could you add that to the test?

================
Comment at: test/CodeGen/Mips/sqrt.ll:12
@@ +11,2 @@
+
+; CHECK: sqrt
----------------
This check needs to be stricter. It will currently match 'sqrt_fn:', 'sqrtf:', etc. in the output even if the intended test fails.

Also, could you put it with the IR tests in test/CodeGen/Mips/llvm-ir/? The test is the same kind of single-IR-statement test as those.


http://reviews.llvm.org/D13235