[PATCH] D41389: [WIP][InstCombine] Missed optimization in math expression: squashing sin(asin), cos(acos)
Hal Finkel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 19 19:31:49 PST 2017
hfinkel added a comment.
In https://reviews.llvm.org/D41389#959975, @efriedma wrote:
> > 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.
================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1273
+ StringRef Name = CallI->getCalledFunction()->getName();
+ if ((II.getIntrinsicID() == Intrinsic::cos && Name == "acos") ||
+ (II.getIntrinsicID() == Intrinsic::sin && Name == "asin")) {
----------------
hfinkel wrote:
> 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)))
>
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.
https://reviews.llvm.org/D41389
More information about the llvm-commits
mailing list