[PATCH] D139130: InstCombine: Fold and (fcmp), (is.fpclass) into is.fpclass

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:829-843
+  if (Mask == fcNan && !IsStrict) {
+    // Equivalent of isnan. Replace with standard fcmp if we don't care about FP
+    // exceptions.
+    Value *FCmp = Builder.CreateIsNaN(Src0);
+    FCmp->takeName(&II);
+    return replaceInstUsesWith(II, FCmp);
+  }
----------------
sepavloff wrote:
> arsenm wrote:
> > arsenm wrote:
> > > sepavloff wrote:
> > > > Replacement of `llvm.is_fpclass` with comparison hides class checks and make analysis of code passes harder. Does it have any benefit over creation of the comparison in selector?
> > > Using fcmp makes analysis easier. fcmp has fewer constraints, and will always be better understood by generic passes than a special case intrinsic. It's a better canonical form; otherwise every single place that considers compares would have to perform the exact same checks for a class that performs a compare.
> > > 
> > > I also just noticed when I rebased I attached the wrong patch here.
> > fcmp also supports fast math flags, class does not
> `is_fpclass` should not be replaced by compare instruction otherwise it can break valid programs. Consider the code:
> ```
> define i1 @check_isnan(float %x) strictfp {
>   %call = call i1 @func_isnan(float %x)
>   ret i1 %call
> }
> 
> define i1 @func_isnan(float %x) "always_inline" {
> entry:
>   %call = i1 call @llvm.is.fpclass.f32(float %x, i32 3)
>   ret i1 %call
> }
> ```
> if InstCombiner replaces the call of `@llvm.is.fpclass.f32` with `fcmp uno`, then during inlining `fcmp` is replaced with a call to `@llvm.experimental.constrained.fcmp.f32`, which has different behavior than `is_fpclass`. 
`is_fpclass` does not produce float value, fast math flags does not make sense for it.


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

https://reviews.llvm.org/D139130