[PATCH] D28314: Change sqrt partial inlining to depend on sqrt argument rather than result.

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

Simon, would you like to commandeer this patch?

I don't really have a strong opinion on the trade-offs here, was just curious about how we ended up with PR31455.



================
Comment at: lib/Transforms/Scalar/PartiallyInlineLibCalls.cpp:45-48
   // v0 = sqrt_noreadmem(src) # native sqrt instruction.
-  // if (v0 is a NaN)
+  // if (src < 0)
   //   v1 = sqrt(src)         # library call.
   // dst = phi(v0, v1)
----------------
sdardis wrote:
> Shouldn't this be:
> 
>    // if (src > 0)
>    //   v0 = sqrt_noreadmem(src) # native sqrt instruction
>    // else
>    //   v1 = sqrt(src) # library call
>    // dst = phi(v0, v1)
The IR-level transformation is actually as described, see the good-prototype.ll test. The sqrt_noreadmem() call gets sunk into the branch later.
Whether this *should* be the case or not is a different issue. :-)


================
Comment at: test/CodeGen/Mips/optimize-fp-math.ll:7
+; 32: c.ult.s $f[[R0:[0-9]+]], $f[[R1:[0-9]+]]
+; 32: sqrt.s $f[[R1]], $f[[R0]]
 ; 64-LABEL: test_sqrtf_float_:
----------------
sdardis wrote:
> This should be:
> 
> ; 32-LABEL: test_sqrtf_float_:
> ; 32: mtc1 $zero, $f[[R0:[0-9]+]]
> ; 32: c.ult.s $f12, $f[[R0]]
> ; 32: bc1t $BB0_[[BB0:[0-9]+]]
> ; 32: sqrt.s $f0, $f12
> ; 32: $BB0_[[BB0]]:
> ; 32: jal sqrtf
> 
> Similarly for the 64 case.
Sure.
The update script doesn't work for MIPS yet, right?


https://reviews.llvm.org/D28314