[PATCH] D157030: InstCombine: Fold out scale-if-denormal pattern

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:3291
+
+static bool matchFMulByZeroIfEqZero(InstCombinerImpl &IC, Value *Cmp0,
+                                    Value *Cmp1, Value *TrueVal,
----------------
`matchFMulByZeroIfResultEqZero`? Seems a lot clearer.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:3427
+      // This pattern appears in code with denormal range checks after it's
+      // assumed denormals are treated as zero. This drops a canonicalization.
+
----------------
What do you mean by "This drops a canonicalization"? Causes us to miss it?

Can you also describe the "scale-if-equals-zero" pattern. Mostly for my sake, not familiar with it. If another reviewer is they might be better fit for this patch.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:3430
+      // TODO: Could relax the signed zero logic. We just need to know the sign
+      // of the result matches, so if we know fmul x, y has the same sign as x.
+      //
----------------
Missing something after the "so"?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:3446
+      if ((Pred == CmpInst::FCMP_ONE || Pred == CmpInst::FCMP_UNE) &&
+          Cmp0 == TrueVal &&
+          matchFMulByZeroIfEqZero(*this, Cmp0, Cmp1, FalseVal, TrueVal, SI,
----------------
Imo:
```
Value * MatchCmp0 = nullptr;
if(Pred == CmpInst::FCMP_OEQ || ...UEQ)
   MatchCmp0 = FalseVal;
else if(Pred == CmpInst::FCMP_ONE || ...UNE)
   MatchCmp0 = TrueVal;
if(Cmp0 == MatchCmp0 && matchFMulByZeroIfEqZero(this, Cmp0, Cmp1, Cmp1, Cmp0, SI))
   return replaceInstUsesWith(SI, Cmp0);
```


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

https://reviews.llvm.org/D157030