Page MenuHomePhabricator

[WIP][InstCombine] Missed optimization in math expression: squashing sin(asin), cos(acos)
Needs ReviewPublic

Authored by Quolyk on Dec 19 2017, 5:34 AM.

Details

Summary

Motivation: https://bugs.llvm.org/show_bug.cgi?id=35603. I have failing test on functions cos_acos_fast and sin_asin_fast. Specifically

%call = call fast double @acos(double %a)
%call = call fast double @asin(double %a)

are not erased after transform, however %call instruction is never used. I'm looking for help, as I do not know why it behaves this way.

Diff Detail

Event Timeline

Quolyk created this revision.Dec 19 2017, 5:34 AM

are not erased after transform, however %call instruction is never used.

asin can set errno, which is a side-effect. For "fast" calls we should probably treat them as dead anyway (in llvm::isMathLibCallNoop), but the issue generally doesn't come up because clang (incorrectly) marks math library calls readnone in fast-math mode.

hfinkel added inline comments.Dec 19 2017, 6:28 PM
lib/Transforms/InstCombine/InstCombineCalls.cpp
1273

Don't match function names like this directly (name matching should go through TLI). Something like this:

LibFunc Func;
auto *Callee = CallI->getCalledFunction();
if (!Callee || !TLI->getLibFunc(*Callee, Func) || !TLI->has(Func))
  return nullptr;

...

        (II.getIntrinsicID() == Intrinsic::sin && LibFunc == LibFunc_asin)) {

...

As it seems like we're likely to get a bunch of these, I recommend adding a matcher for TLI functions so that we can just do:

match(Src, m_LibCall<LibFunc_asin>(TLI, m_Value(V)))

are not erased after transform, however %call instruction is never used.

asin can set errno, which is a side-effect. For "fast" calls we should probably treat them as dead anyway (in llvm::isMathLibCallNoop), but the issue generally doesn't come up because clang (incorrectly) marks math library calls readnone in fast-math mode.

I've been guilty of saying this too, but it's not completely true. On systems (e.g., MacOS) where the math calls don't set errno ever, this is obviously fine. On systems (e.g., Linux) where the regular math calls might set errno, many of the calls never actually set errno under finite-math assumptions (e.g., sin and cos), and many others are redirected by /usr/include/bits/math-finite.h to __<name>_finite variants which don't. Thus, it's actually okay to set these as readnone (because the compiler (by setting FINITE_MATH_ONLY), system headers, and implementation conspire to make this correct).

In general, we should probably restrict these transformations which remove calls to cases where the calls are readnone to be safe.

lib/Transforms/InstCombine/InstCombineCalls.cpp
1273

And, also, you should check for LibFunc_asin_finite. And, also, to catch non-double-precision library calls, also asinf, asinf_finite, asinl, asinl_finite. Please add tests for these.