[PATCH] D41342: [InstCombine] Missed optimization in math expression: simplify calls exp functions

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: test/Transforms/InstCombine/fmul-exp.ll:30-31
+;
+  %1 = call fast double @llvm.exp.f64(double %a)
+  %2 = call fast double @llvm.exp.f64(double %b)
+  %mul = fmul fast double %1, %2
----------------
lebedev.ri wrote:
> I think this can still be transformed, as long as at least one of them has only one use.
> This would be transformed into
> ```
>   %2 = call fast double @llvm.exp.f64(double %b)
>   call void @use(double %2)
>   %t1 = add fast double %a, %b
>   %t2 = call fast double @llvm.exp.f64(double %t2)
>   ret double %t2
> ```
> which has the same number of instruction,
> And add a test where both of them are two-use, which won't be transformed.
This comment has not been addressed. I agree with Roman - if there's only 1 extra use, it makes sense to reduce the dependency chain and reduce fmul to fadd.


================
Comment at: test/Transforms/InstCombine/fmul-exp.ll:62
+;
+  %tmp = call fast double @llvm.exp.f64(double %a)
+  %tmp1 = call fast double @llvm.exp.f64(double %b)
----------------
Quolyk wrote:
> spatel wrote:
> > We don't need the full "fast" to enable these transforms. Can you adjust the tests to show the minimal amount of FMF necessary to allow the optimizations? We need "reassoc" + anything else?
> > 
> > Do you have commit access? If so, please commit these files to trunk with the current (without this code patch) output.
> I have commit access, but I don't want to commit without patch approval. Revision is fixed according to your comments.
Thanks. I think the patch is close to approval, so you should commit the tests now. There are 2 benefits to committing the tests first:
1. You and the reviewers can see the actual diffs that will occur with the code change. Ie, if something in a test is not already in canonical form, you can correct or show that in the baseline test commit.
2. If the code change must be reverted for some reason, we should not lose the regression test coverage because of that revert.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D41342