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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm 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:
> sepavloff wrote:
> > 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.
> Fast math flags apply to the inputs and outputs, which is one of the problems with them. fcmp nnan can trivially fold to true/false for example
How is it different? If there's a difference in behavior, there's an inliner bug. We have separate constrained.fcmp and constrained.fcmps for compares which may or may not signal


================
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:
> 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.
Fast math flags apply to the inputs and outputs, which is one of the problems with them. fcmp nnan can trivially fold to true/false for example


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

https://reviews.llvm.org/D139130