[PATCH] D41322: [InstCombine] Missed optimization in math expression: squashing sqrt functions

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:626
 
   bool AllowReassociate = I.isFast();
 
----------------
Note that we have a fast-math-flag specifically for reassociation now (as well as a FMF for approximation of math functions). I'm not sure what combination would be needed for this transform. It's independent of this patch, but something to think about.


================
Comment at: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:731
 
+  // sqrt(a) * sqrt(b) -> sqrt(a * b)
+  if (AllowReassociate) {
----------------
What if sqrt(a) or sqrt(b) has another use besides the multiply? I don't think we want to do the transform in that case. Either way, please add a test for that kind of example.


================
Comment at: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:733-739
+    IntrinsicInst *I0 = dyn_cast<IntrinsicInst>(Op0);
+    IntrinsicInst *I1 = dyn_cast<IntrinsicInst>(Op1);
+    if (I0 && I1 &&
+        I0->getIntrinsicID() == Intrinsic::sqrt &&
+        I1->getIntrinsicID() == Intrinsic::sqrt) {
+      Value *Opnd0 = I0->getOperand(0);
+      Value *Opnd1 = I1->getOperand(0);
----------------
You can use:
  match(Op0, m_Intrinsic<Intrinsic::sqrt>( ...
to simplify this code.


================
Comment at: test/Transforms/InstCombine/fmul.ll:185-199
+; CHECK-LABEL @sqrt_a_sqrt_b(
+; CHECK: fmul fast double %a, %b
+; CHECK: call fast double @llvm.sqrt.f64(double %1)
+define double @sqrt_a_sqrt_b(double %a, double %b) {
+  %1 = call fast double @llvm.sqrt.f64(double %a)
+  %2 = call fast double @llvm.sqrt.f64(double %b)
+  %mul = fmul fast double %1, %2
----------------
Please use the script at utils/update_test_checks.py to auto-generate the checks for any tests you add. The checks will be more exact and have regex for the variable names, so they'll be more robust.


https://reviews.llvm.org/D41322