[PATCH] D137811: InstCombine: Port amdgcn.class intrinsic combines to is.fpclass

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:819
+  const ConstantInt *CMask = cast<ConstantInt>(Src1);
+  uint32_t Mask = CMask->getZExtValue();
+  const bool IsStrict = II.isStrictFP();
----------------
jcranmer-intel wrote:
> It feels like you should be able to use ninf/nnan/nsz flags to modify the mask here to make it more likely to fall into one of these categories, although this may provoke some backlash from those who argue that `nnan isnan(x)` => unconditional `false` shouldn't be an allowable optimization.
I think this is debatable, and beyond the scope of this patch where I'd like to simply move what we already do for the target specific intrinsic 


================
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);
----------------
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


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:837
+  }
+
+  // fp_class (nnan x), qnan|snan|other -> fp_class (nnan x), other
----------------
jcranmer-intel wrote:
> You're also missing cases for `Mask == fcPosInf` and `Mask == fcNegInf`, which can similarly be lowered to quiet comparisons.
Ditto, for now I'd like to just move the code 


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:845
+  const ConstantFP *CVal = dyn_cast<ConstantFP>(Src0);
+  if (!CVal) {
+#if 0
----------------
foad wrote:
> What's this `!CVal` check for?
I moved this part to InstSimplify, will drop 


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:818-829
+  const ConstantInt *CMask = dyn_cast<ConstantInt>(Src1);
+  if (!CMask) {
+    if (isa<UndefValue>(Src0)) {
+      return replaceInstUsesWith(II, UndefValue::get(II.getType()));
+    }
+
+    if (isa<UndefValue>(Src1)) {
----------------
sepavloff wrote:
> The second argument is described with `ImmArg`. It must always be a constant.
I'm debating removing this restriction 


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

https://reviews.llvm.org/D137811