Here we have implemented the following missing trigonometric optimizations.

- tan(x)*cos(x)=sin(x)
- sin(x)*cos(x) = sin(2*x)/2
- sin(x)/tan(x)=cos(x);
- tan(x)/sin(x)=1/cos(x);

Here is the reference to the missing optimization reported on bugzilla.

Differential D41659
Implementing missing trigonometric optimizations cs15btech11041 on Jan 2 2018, 12:53 AM.
Details - Reviewers
majnemer craig.topper davide scanon escha
Here we have implemented the following missing trigonometric optimizations. - tan(x)*cos(x)=sin(x)
- sin(x)*cos(x) = sin(2*x)/2
- sin(x)/tan(x)=cos(x);
- tan(x)/sin(x)=1/cos(x);
Here is the reference to the missing optimization reported on bugzilla.
Diff Detail - Repository
- rL LLVM
## Event TimelineComment Actions As mentioned in llvm-dev, it looks like David Majnemer and Craig Topper might be appropriate reviewers for this patch (or can perhaps help to suggest other reviewers). It looks like you've run clang-format on the entire lib/Transforms/InstCombine/InstructionCombining.cpp file. This makes it harder to review the changes you've made. Best practice is to only clang-format lines that you touch. I use the git-clang-format helper script for this, and it looks like clang-format-diff.py can help if you're using SVN: https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format Comment Actions Thanks for your patch! Usually it is easier to review smaller patches. Splitting it up into 4 patches for the 4 different cases you implemented would make it slightly easier to reason about the 4 unrelated transformation in isolation. Comment Actions Meta point: It's unclear to me whether this is something profitable to implement. I looked and it seems other compilers (most notably, GCC) don't implement at least the first transformation you implemented (I didn't bother to check the others). tl;dr: what's your motivation? Comment Actions Something else you may want to keep in mind is that these transformations may introduce dramatic rounding errors, so they need some thoughts/analysis before they can go in (cc: @scanon), even under Comment Actions Updates made: - Made the test cases more modular as mentioned in the comments.
- Instead of clang format on entire InstructionCombine.cpp clang-formatted only the modified/inserted code for better understanding.
3.Comments improved. Comment Actions Hi, thanks for the comment. Thanks for adding appropriate reviewers for the patch.
As suggested, I have updated the diff file with clang format only on the section of code modified/inserted. Hopefully it is easier for the review. Thanks for the suggestion.
Comment Actions Yes. Thank you for the suggestions. I have updated the diff to incorporate the changes you suggested. And submitted different test case for each optimization in isolation. Hope it has made it easier to review the patch. Comment Actions I have added FastMathFlagGuard Guard to the IRBuilder. And also copied the ffast-math flags of the division/multiplication instruction. To the new trigonometric/multiplication/division instruction inserted. As for precision, we are replacing two trigonometric functions with single equivalent trigonometric function with fast math flag guards. Also the type which we are specifying in CreateCall is same as that of the division/multiplication instruction. Hence would have the same type and precision. Please let me know if I did not answer your question. Comment Actions Hi, thanks for the comments. the motivation for taking this up were two things. https://bugs.llvm.org/show_bug.cgi?id=35602 Second, replacing two trigonometric operations with single trigonometric operation or with a mul/div instruction would save considerable clock cycles. It says trigonometric operations require almost five time the clock cycles needed by multiplication/division.Also to add we are reducing the code size. As one could see from the test cases, the number of instructions after transformations <= number of instructions before. And to that we are also saving clock cycles. https://stackoverflow.com/questions/2479517/is-trigonometry-computationally-expensive Comment Actions Added the right file for sin_div_tan.ll test case (incorrect was added in previous commit). Comment Actions Strongly agree. I understand there's a common theme here, but it will save time making redundant comments if we start with a patch for just one of these transforms. The structure of the patch is not correct. We don't need a trig helper function because it's impossible for an integer op like mul or udiv to have an FP operand like llvm.sin. Comment Actions
I think I wasn't too clear, sorry. I meant splitting up the whole patch into 4 different patches and submitting them as separate reviewss, i.e. one review only including the code & test for This also makes it slightly easier to decide if each transform is beneficial on a case-by-case basis. |