[PATCH] D112675: [Instcombine] Add patterns to generate fneg(fabs(x)) instead of fcmp/sub/selects (part 1)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

Thanks for splitting this up and creating the Alive link!

I'm still not clear if the FMF propagation is behaving as well as it could. For example, if the select has nsz, then can it always be added to the fneg?
https://alive2.llvm.org/ce/z/2Qq6Pz

It would also help to sprinkle some extra flags like nnan and ninf into the tests, so we can tell how those are modified even though they are not required for the transform.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2968
+  // 2. nsz on the sub (canonicalized to FNeg) or on the select:
+  //    (X < +/-0.0) ? X : -X         --> -X
+  if (match(CondVal, m_FCmp(Pred, m_Specific(TrueVal), m_AnyZeroFP())) &&
----------------
"--> -X" should be "--> -fabs(X)"


================
Comment at: llvm/test/Transforms/InstCombine/fneg-fabs.ll:28
 ;
   %cmp = fcmp ult double %x, 0.000000e+00
   %negX = fsub double 0.000000e+00, %x
----------------
mnadeem wrote:
> arsenm wrote:
> > Missing some cases where the compared value is -0?
> Any specific cases? Or should I duplicate all tests and set the compared value as `-0`?
We canonicalize fcmp to +0.0, so I think it would be enough to just switch any one of these tests to confirm that.
Using different types (a vector, a float, a half, etc) as we cycle through these would also increase coverage.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112675/new/

https://reviews.llvm.org/D112675