[PATCH] D137811: InstCombine: Perform basic isnan combines on llvm.is.fpclass

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:2437
 
+  /// Return a boolean value testing if \p Arg is a NaN.
+  Value *CreateIsNaN(Value *Arg, const Twine &Name = "") {
----------------
I don't think this function should be added -- it emits the wrong thing for strictfp mode, but also just doesn't seem useful.

That we sometimes canonicalize to fcmp is just an implementation detail -- the CreateFCmpUNO call can just be done inline in foldIntrinsicIsFPClass.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:824
+    // Equivalent of isnan. Replace with standard fcmp.
+    Value *FCmp = Builder.CreateFCmpUNO(Src0, Src0);
+    FCmp->takeName(&II);
----------------
arsenm wrote:
> jyknight wrote:
> > arsenm wrote:
> > > sepavloff wrote:
> > > > Is it profitable to make such replacement early? Is there any advantage, at least hypothetical? 
> > > I think an fcmp should be more canonical and better handled by existing optimizations, than class which is handled ~nowhere
> > This seems OK.
> > 
> > I'm not sure it's the //best// choice -- if a CPU actually has an fpclassify instruction, is it really a good idea to canonicalize in generic code to fcmp? But I think that's fine to revisit later if it becomes a problem.
> I've been thinking if we can do a classify in <= 2 IR instructions, fcmp+fabs is probably a better canonical form. If a class pattern is 3-4+ instructions, the class is probably better. FCmp + fneg + fabs are always going to be more broadly understood. Fcmp also supports fast math flags, unlike class (I guess we could fix that though)
That may be true. However, on architectures that have an fpclass instruction, it could be profitable to canonicalize other operations into llvm.is.fpclass operations, especially if that then allows merging multiple llvm.is.fpclass calls into one.

E.g. `bool a = isinf(x) || isnan(x)` can turn into a single instruction `llvm.is.fpclass(x, snan/qnan/pinf/ninf)`.

(However, this isn't an objection to taking this patch for now -- revisiting the canonical form later is always possible.)


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

https://reviews.llvm.org/D137811