[mips][microMIPS] Fix an issue with selecting sqrt instruction in LLVM backend

[zbuljan]

Summary

7 tests failed during fast LLVM test-suite run:

• MultiSource/Benchmarks/McCat/18-imp/imp
• MultiSource/Applications/oggenc/oggenc
• MultiSource/Benchmarks/MallocBench/gs/gs
• MultiSource/Benchmarks/MiBench/automotive-susan/automotive-susan
• MultiSource/Benchmarks/VersaBench/beamformer/beamformer
• MultiSource/Benchmarks/MiBench/consumer-lame/consumer-lame
• MultiSource/Benchmarks/Bullet/bullet

Error message was in the form of:
fatal error: error in backend: Cannot select: 0x95c3288: f32 = fsqrt 0x95c0190 [ORD=9] [ID=18]

0x95c0190: f32 = fadd 0x95bef30, 0x95c4d00 [ORD=8] [ID=17]
  0x95bef30: f32 = fmul 0x95c4988, 0x95c4988 [ORD=5] [ID=16]

...

There was problem with selecting sqrt instruction in LLVM backend.

To fix the issue changes are made in TableGen definition for sqrt instruction in MipsInstrFPU.td and new test file sqrt.ll is added to LLVM regression tests.

[dsanders]

[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.

lib/Target/Mips/MipsInstrFPU.td

356–357 ↗

(On Diff #35944)

I notice you've added StdMMR6Rel but your test doesn't try microMIPS32R6. Could you add that to the test?

test/CodeGen/Mips/sqrt.ll

12 ↗

(On Diff #35944)

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.

[zbuljan]

Added new RUN line for microMIPS32R6 test.
Added CHECK-NO to restrict maching of sqrt.
sqrt.ll is moved to test/CodeGen/Mips/llvm-ir folder.

[dsanders]

Thanks. The new commit message is a lot better but I need to re-iterate one important point.

Problem was reported on bugzilla as bug 2206.

Most people would read this as referring to https://llvm.org/PR2206 but the solution isn't to link to our internal bugzilla. The key point was that our internal bugzilla tickets mean nothing to the wider LLVM community. Please drop this sentence.

---

LGTM with our internal bugzilla reference removed and the test case fixed.

test/CodeGen/Mips/llvm-ir/sqrt.ll

13–15 ↗

(On Diff #36337)

The CHECK-NOT doesn't quite achieve what you intended. Consider:

sqrt_fn:
  ...
  jal sqrtf

The CHECK-LABEL will match the sqrt_fn: as intended, the CHECK will match the 'sqrt' in 'jal sqrtf'. After these two matches are made, the CHECK-NOT will check that sqrtf doesn't match between the other two matches. This will successfully find zero matches and the test will mistakenly pass.

I'd just add the registers to the instruction I want to match like so:

; CHECK: sqrt $f0, $f12

with that, no CHECK-LABEL or CHECK-NOT is necessary.

[zbuljan]

test/CodeGen/Mips/llvm-ir/sqrt.ll

13–15 ↗

(On Diff #36337)

In this case, I think, there should be sqrt.s in expression:
; CHECK: sqrt.s $f0, $f12

[dsanders]

In this case, I think, there should be sqrt.s in expression:

I agree.

[zbuljan]

Removed summary line that references bugzilla.
Updated CHECK line with sqrt instruction: chaged to sqrt.s and added registers.
Removed unnecessary CHECK lines from .ll file.

[dsanders]

LGTM

llvm/trunk/lib/Target/Mips/MipsInstrFPU.td

 
 def FABS_S : MMRel, ABSS_FT<"abs.s", FGR32Opnd, FGR32Opnd, II_ABS, fabs>,
              ABSS_FM<0x5, 16>;
 def FNEG_S : MMRel, ABSS_FT<"neg.s", FGR32Opnd, FGR32Opnd, II_NEG, fneg>,
              ABSS_FM<0x7, 16>;
 defm FABS : ABSS_M<"abs.d", II_ABS, fabs>, ABSS_FM<0x5, 17>;
 defm FNEG : ABSS_M<"neg.d", II_NEG, fneg>, ABSS_FM<0x7, 17>;
 
-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;
 
 // The odd-numbered registers are only referenced when doing loads,
 // stores, and moves between floating-point and integer registers.
 // When defining instructions, we reference all 32-bit registers,
 // regardless of register aliasing.
 
 /// Move Control Registers From/To CPU Registers

llvm/trunk/test/CodeGen/Mips/llvm-ir/sqrt.ll

+; RUN: llc < %s -march=mipsel -mcpu=mips32r2 -mattr=+micromips | FileCheck %s
+; RUN: llc < %s -march=mips -mcpu=mips32r2 -mattr=+micromips | FileCheck %s
+; RUN: llc < %s -march=mips -mcpu=mips32r6 -mattr=+micromips | FileCheck %s
+
+define float @sqrt_fn(float %value) #0 {
+entry:
+  %sqrtf = tail call float @sqrtf(float %value) #0
+  ret float %sqrtf
+}
+
+declare float @sqrtf(float)
+
+; CHECK: sqrt.s $f0, $f12