[PATCH] D41286: [InstCombine] Missed optimization in math expression: sin(x) / cos(x) => tan(x)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:560-563
+static StringRef libFuncNameForUnaryFloatFn(const TargetLibraryInfo *TLI,
+                                            Type *Ty, LibFunc DoubleFn, 
+                                            LibFunc FloatFn, 
+                                            LibFunc LongDoubleFn) {
----------------
Can you move and reuse the very similar function that's currently in SimplifyLibCalls.cpp?
  static bool hasUnaryFloatFn(const TargetLibraryInfo *TLI, Type *Ty,
                            LibFunc DoubleFn, LibFunc FloatFn,
                            LibFunc LongDoubleFn) {

See also:
  /// Emit a call to the unary function named 'Name' (e.g.  'floor'). This
  /// function is known to take a single of type matching 'Op' and returns one
  /// value with the same type. If 'Op' is a long double, 'l' is added as the
  /// suffix of name, if 'Op' is a float, we add a 'f' suffix.
  Value *emitUnaryFloatFnCall(Value *Op, StringRef Name, IRBuilder<> &B,
                              const AttributeList &Attrs);

...in BuildLibCalls.h


================
Comment at: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1495-1498
+    Value *CosArg;
+    if (match(Op0, m_Intrinsic<Intrinsic::sin>(m_Value(SinArg))) &&
+        match(Op1, m_Intrinsic<Intrinsic::cos>(m_Value(CosArg))) &&
+        SinArg == CosArg) {
----------------
Use m_Specific to simplify this:
   // sin(a) / cos(a) -> tan(a)
   Value *A;
   if (match(Op0, m_Intrinsic<Intrinsic::sin>(m_Value(A))) &&
       match(Op1, m_Intrinsic<Intrinsic::cos>(m_Specific(A)))) {


================
Comment at: test/Transforms/InstCombine/fdiv.ll:90-92
+  %1 = call fast double @llvm.sin.f64(double %a)
+  %2 = call fast double @llvm.cos.f64(double %a)
+  %div = fdiv fast double %1, %2
----------------
Please vary the fast-ness in these tests or add test(s) that show some variation. I'd prefer to see at least one test where we show the minimum case as we showed in one of the related patches - only the fdiv has relaxed math while the trig calls are strict.


https://reviews.llvm.org/D41286