[PATCH] D72319: [InstCombine] Adding Z / (1.0 / Y) => (Y * Z)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1249
+    if (match(Op1, m_FDiv(m_SpecificFP(1.0), m_Value(Y))) &&
+        (!isa<Constant>(Y) || !isa<Constant>(Op0)))
+      return BinaryOperator::CreateFMulFMF(Y, Op0, &I);
----------------
I don't think these constant checks are necessary. (I'm not sure why they are part of the existing code either - maybe some corner case with denorm values?)

If Y is a constant, 1.0/C will get constant folded and Z/C' will eventually get folded too, so this should just get to canonical form quicker. The 'Z' is constant pattern is similar. 

FDiv with constant operand are generally handled within:
foldFDivConstantDivisor()
foldFDivConstantDividend()


================
Comment at: llvm/test/Transforms/InstCombine/fdiv-to-fmul-opt.ll:15-21
+define dso_local double @fun() #0 {
+  %r1 = alloca double, align 8
+  %r2 = alloca double, align 8
+  %r3 = alloca double, align 8
+  %r4 = alloca double, align 8
+  %r5 = alloca double, align 8
+  %r6 = alloca double, align 8
----------------
1. Add a test within test/Transforms/InstCombine/fdiv.ll instead of creating a new file.
2. Create minimal test(s) to show the transform; in particular, this transform does not require any of alloca, load, store, so the test shouldn't have those.
3. Use this script to auto-generate the CHECK lines:
utils/update_test_checks.py
4. I prefer that you add the test *without* this code patch showing the current IR produced as a preliminary patch. That way, we will see the diff from the new transform when this patch is committed.
5. If you need help committing/pushing the patches, let me know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72319