[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-835
+  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);
+  }
----------------
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`. 


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

https://reviews.llvm.org/D139130