[Bug jit/64020] New: jit misses sin/cos optimizations

[Bug jit/64020] New: jit misses sin/cos optimizations

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64020

            Bug ID: 64020
           Summary: jit misses sin/cos optimizations
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: jit
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: dmalcolm at gcc dot gnu.org

When running testsuite/jit.dg/test-functions.c under valgrind I noticed that
tree-ssa-math-opts.c:execute_cse_sincos_1 was bailing out at this conditional:

      fndecl = mathfn_built_in (type, BUILT_IN_CEXPI);
      if (!fndecl)
        return false;

(which is actually a memory leak; I'll submit a fix for that as part of PR
jit/63854)

TODO: Why does this fail, and is this leading to missed optimizations in the
jitted code?

[Bug jit/64020] jit misses sin/cos optimizations

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64020

--- Comment #1 from dmalcolm at gcc dot gnu.org ---
builtin_info.decl is NULL for BUILT_IN_CEXPI.

4586    static inline bool
4587    builtin_decl_implicit_p (enum built_in_function fncode)
4588    {
4589      size_t uns_fncode = (size_t)fncode;
4590    
4591      gcc_checking_assert (BUILTIN_VALID_P (fncode));
4592      return (builtin_info.decl[uns_fncode] != NULL_TREE
4593          && builtin_info.implicit_p[uns_fncode]);
4594    }
4595    

Looks like I may need to do something like what the other frontends do to
register builtins (see. e.g. c-family/c-common.c:c_define_builtins)

[Bug jit/64020] jit misses sin/cos optimizations

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64020

--- Comment #2 from dmalcolm at gcc dot gnu.org ---
For reference, both at -O3, based on r217939:

./cc1 of

#include <math.h>

int
test_of_builtin_trig (double theta)
{
  return 2 * sin (theta) * cos (theta);
}

generates:

test_of_builtin_trig:
.LFB3:
        .cfi_startproc
        subq    $24, %rsp
        .cfi_def_cfa_offset 32
        leaq    8(%rsp), %rdi
        movq    %rsp, %rsi
        call    sincos
        movsd   8(%rsp), %xmm0
        addsd   %xmm0, %xmm0
        mulsd   (%rsp), %xmm0
        addq    $24, %rsp
        .cfi_def_cfa_offset 8
        cvttsd2si       %xmm0, %eax
        ret
        .cfi_endproc

[would it be even better to optimize this to just a sin (2 * theta) ?]

whereas the jit test generates:

test_of_builtin_trig:
.LFB32:
    .cfi_startproc
.LVL4:
.L37:
    subq    $24, %rsp
    .cfi_def_cfa_offset 32
    movsd    %xmm0, 8(%rsp)
    call    sin@PLT
.LVL5:
    movsd    8(%rsp), %xmm1
    movsd    %xmm0, (%rsp)
    movapd    %xmm1, %xmm0
    call    cos@PLT
.LVL6:
    mulsd    (%rsp), %xmm0
    addq    $24, %rsp
    .cfi_def_cfa_offset 8
    addsd    %xmm0, %xmm0
    ret
    .cfi_endproc

[Bug jit/64020] jit misses sin/cos optimizations

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64020

--- Comment #3 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
On Mon, 24 Nov 2014, dmalcolm at gcc dot gnu.org wrote:

> [would it be even better to optimize this to just a sin (2 * theta) ?]

Only with -funsafe-math-optimizations (it's unsafe for large arguments 
where 2 * theta overflows, and that transformation can also lose underflow 
exceptions for small arguments where sin (theta) is in the tiny range but 
sin (2 * theta) isn't).

[Bug jit/64020] jit misses sin/cos optimizations

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64020

dmalcolm at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2014-11-25
     Ever confirmed|0                           |1

--- Comment #4 from dmalcolm at gcc dot gnu.org ---
Am testing a candidate patch for this

[Bug jit/64020] jit misses sin/cos optimizations

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64020

--- Comment #5 from dmalcolm at gcc dot gnu.org ---
(In reply to dmalcolm from comment #4)
> Am testing a candidate patch for this

Patch posted as:
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg03229.html

[Bug jit/64020] jit misses sin/cos optimizations

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64020

--- Comment #6 from dmalcolm at gcc dot gnu.org ---
Author: dmalcolm
Date: Mon Dec  1 18:23:37 2014
New Revision: 218240

URL: https://gcc.gnu.org/viewcvs?rev=218240&root=gcc&view=rev
Log:
PR jit/64020: Fixes to handling of builtins

    PR jit/64020
    * docs/topics/types.rst (Standard types) Add new enum values to
    the table of enum gcc_jit_types: GCC_JIT_TYPE_COMPLEX_FLOAT,
    GCC_JIT_TYPE_COMPLEX_DOUBLE, GCC_JIT_TYPE_COMPLEX_LONG_DOUBLE.
    Widen the left-hand column so that
    GCC_JIT_TYPE_COMPLEX_LONG_DOUBLE will fit.
    * docs/_build/texinfo/libgccjit.texi: Regenerate.

    * jit-builtins.c: Include stringpool.h and jit-playback.h.
    Move everything out of the gcc::jit::recording namespace into
    just gcc::jit.
    (struct builtin_data): Add fields "fnclass", "attr", and
    "implicit_p".
    (DEF_BUILTIN): Update macro so populate the new fields.
    (builtins_manager::builtins_manager): Update for move out of
    recording namespace.  Initialize the m_attributes array.
    (builtins_manager::get_builtin_function): Likewise.
    (builtins_manager::get_builtin_function_by_id): New function.
    (builtins_manager::make_builtin_function): Update for move out of
    recording namespace.  Add fix for PR jit/64020 by detecting
    specific builtin ids and having them ensure that builtins for
    other ids are created as necessary.
    (builtins_manager::get_type): Update for move out of recording
    namespace.
    (builtins_manager::make_type): Likewise.  Add some missing
    #undefs.
    (builtins_manager::make_primitive_type): Update for move out of
    recording namespace.  Implement the three BT_COMPLEX_ cases and
    BT_DOUBLE_PTR.
    (builtins_manager::make_fn_type): Update for move out of recording
    namespace.
    (builtins_manager::make_ptr_type): Likewise.
    (builtins_manager::finish_playback): New function.
    (builtins_manager::get_class): New function.
    (builtins_manager::implicit_p): New function.
    (builtins_manager::get_attrs_tree): Two new functions.
    (builtins_manager::make_attrs_tree): New function.

    * jit-builtins.h: Move everything out of the gcc::jit::recording
    namespace into just gcc::jit.
    (enum built_in_attribute): New.
    (builtins_manager::builtins_manager): Update decl for namespace
    change.
    (builtins_manager::get_builtin_function): Likewise.
    (builtins_manager::get_class): New.
    (builtins_manager::implicit_p): New.
    (builtins_manager::get_attrs_tree): Two new functions.
    (builtins_manager::make_attrs_tree): New function.
    (builtins_manager::finish_playback): New.
    (builtins_manager::get_builtin_function_by_id): New.
    (builtins_manager::make_builtin_function): Update decl for
    namespace change.
    (builtins_manager::get_type): Likewise.
    (builtins_manager::make_type): Likewise.
    (builtins_manager::make_primitive_type): Likewise.
    (builtins_manager::make_fn_type): Likewise.
    (builtins_manager::make_ptr_type): Likewise.
    (builtins_manager): Likewise for fields.  Add new field
    "m_attributes".

    * jit-common.h (NUM_GCC_JIT_TYPES): Update.
    (builtins_manager): Update forward decl to reflect namespace
    change.

    * jit-playback.c: Include attribs.h and jit-builtins.h.
    (gcc::jit::playback::context::get_tree_node_for_type): Add cases
    for the new COMPLEX_ types.
    (gcc::jit::playback::context::new_function): If creating a
    builtin, set the DECL_BUILT_IN_CLASS and attributes on the fndecl,
    and call set_builtin_decl.
    (gcc::jit::playback::context::replay): If we have a
    builtins_manager, call its finish_playback method when we're done.

    * jit-playback.h:
    (gcc::jit::playback::context::get_builtins_manager): New function.

    * jit-recording.c
    (gcc::jit::recording::context::get_builtins_manager): New function.
    (gcc::jit::recording::get_builtin_function): Use
    get_builtins_manager, in case we're a child context.
    (gcc::jit::recording::memento_of_get_type::dereference): Add the
    COMPLEX_ types.
    (gcc::jit::recording::memento_of_get_type::is_int): Likewise.
    (gcc::jit::recording::memento_of_get_type::is_float): Likewise.
    (gcc::jit::recording::memento_of_get_type::is_bool): Likewise.
    (get_type_strings): Likewise.

    * jit-recording.h
    (gcc::jit::recording::context::get_builtins_manager): New.

    * libgccjit.h (enum gcc_jit_types): Add
    GCC_JIT_TYPE_COMPLEX_FLOAT, GCC_JIT_TYPE_COMPLEX_DOUBLE,
    GCC_JIT_TYPE_COMPLEX_LONG_DOUBLE.


Modified:
    trunk/gcc/jit/ChangeLog
    trunk/gcc/jit/docs/_build/texinfo/libgccjit.texi
    trunk/gcc/jit/docs/topics/types.rst
    trunk/gcc/jit/jit-builtins.c
    trunk/gcc/jit/jit-builtins.h
    trunk/gcc/jit/jit-common.h
    trunk/gcc/jit/jit-playback.c
    trunk/gcc/jit/jit-playback.h
    trunk/gcc/jit/jit-recording.c
    trunk/gcc/jit/jit-recording.h
    trunk/gcc/jit/libgccjit.h

[Bug jit/64020] jit misses sin/cos optimizations

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64020

--- Comment #7 from dmalcolm at gcc dot gnu.org ---
(In reply to dmalcolm from comment #5)
> (In reply to dmalcolm from comment #4)
> > Am testing a candidate patch for this
> 
> Patch posted as:
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg03229.html

Committed as r218240.

Should be fixed as of that commit, but the testcase does not yet automatically
verify the fix.  I'll keep this open until it does (which will require
implementing a good way to do that).

[Bug jit/64020] jit misses sin/cos optimizations

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64020

--- Comment #8 from dmalcolm at gcc dot gnu.org ---
(In reply to dmalcolm from comment #7)
> (In reply to dmalcolm from comment #5)
> > (In reply to dmalcolm from comment #4)
> > > Am testing a candidate patch for this
> > 
> > Patch posted as:
> > https://gcc.gnu.org/ml/gcc-patches/2014-11/msg03229.html
> 
> Committed as r218240.
> 
> Should be fixed as of that commit, but the testcase does not yet
> automatically verify the fix.  I'll keep this open until it does (which will
> require implementing a good way to do that).

I've opened PR jit/64166 to track providing a way to write such a verification.

[Bug jit/64020] jit misses sin/cos optimizations

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64020

--- Comment #9 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Author: dmalcolm
Date: Tue Dec  9 15:35:39 2014
New Revision: 218521

URL: https://gcc.gnu.org/viewcvs?rev=218521&root=gcc&view=rev
Log:
PR jit/64166: Add API entrypoint gcc_jit_context_enable_dump

gcc/jit/ChangeLog:
    PR jit/64166
    * docs/topics/contexts.rst (Debugging): Add description of
    gcc_jit_context_enable_dump.
    * docs/_build/texinfo/libgccjit.texi: Regenerate.
    * jit-playback.c: Include context.h.
    (class auto_argvec): New class.
    (auto_argvec::~auto_argvec): New function.
    (gcc::jit::playback::context::compile): Convert fake_args to be
    an auto_argvec, so that it can contain dynamically-allocated
    strings.   Construct a vec of all requested dumps, and pass it to
    make_fake_args.  Extract requested dumps between the calls to
    toplev::main and toplev::finalize.
    (gcc::jit::playback::context::make_fake_args): Convert param
    "argvec" to be a vec <char *>, and gain a "requested_dumps"
    param.  Convert to dynamically-allocated arg strings by converting
    ADD_ARG to take a copy of the arg, and add ADD_ARG_TAKE_OWNERSHIP
    for args that are already a copy.  Add args for all requested dumps.
    (gcc::jit::playback::context::extract_any_requested_dumps): New
    function.
    (gcc::jit::playback::context::read_dump_file): New function.
    * jit-playback.h (gcc::jit::playback::context::make_fake_args):
    Convert param "argvec" to be a vec <char *>, and gain a
    "requested_dumps" param.
    (gcc::jit::playback::context::extract_any_requested_dumps): New
    function.
    (gcc::jit::playback::context::read_dump_file): New function.
    * jit-recording.c (gcc::jit::recording::context::enable_dump): New
    function.
    (gcc::jit::recording::context::get_all_requested_dumps): New
    function.
    * jit-recording.h (gcc::jit::recording::requested_dump): New
    struct.
    (gcc::jit::recording::context::enable_dump): New function.
    (gcc::jit::recording::context::get_all_requested_dumps): New
    function.
    (gcc::jit::recording::context::m_requested_dumps): New field.
    * libgccjit.c (gcc_jit_context_enable_dump): New API entrypoint.
    * libgccjit.h (gcc_jit_context_enable_dump): New API entrypoint.
    * libgccjit.map (gcc_jit_context_enable_dump): New API entrypoint.

gcc/testsuite/ChangeLog:
    PR jit/64166
    PR jit/64020
    * jit.dg/harness.h (CHECK_STRING_CONTAINS): New macro.
    (check_string_contains): New function.
    * jit.dg/test-error-unrecognized-dump.c: New file.
    * jit.dg/test-functions.c (trig_sincos_dump): New variable.
    (trig_statistics_dump): New variable.
    (create_test_of_builtin_trig): Enable dumping of "sincos" and
    "statistics" into "trig_sincos_dump" and "trig_statistics_dump".
    (verify_test_of_builtin_trig): Verify the sincos and statistics
    dumps.
    * jit.dg/test-sum-of-squares.c (dump_vrp1): New variable.
    (create_code): Enable dumping of "tree-vrp1" into dump_vrp1.
    (verify_code): Verify the tree-vrp1 dump.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/jit/docs/_build/texinfo/libgccjit.texi
    trunk/gcc/jit/docs/topics/contexts.rst
    trunk/gcc/jit/jit-playback.c
    trunk/gcc/jit/jit-playback.h
    trunk/gcc/jit/jit-recording.c
    trunk/gcc/jit/jit-recording.h
    trunk/gcc/jit/libgccjit.c
    trunk/gcc/jit/libgccjit.h
    trunk/gcc/jit/libgccjit.map
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/jit.dg/harness.h
    trunk/gcc/testsuite/jit.dg/test-functions.c
    trunk/gcc/testsuite/jit.dg/test-sum-of-squares.c

[Bug jit/64020] jit misses sin/cos optimizations

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64020
Bug 64020 depends on bug 64166, which changed state.

Bug 64166 Summary: JIT does not provide a way for verifying dumpfiles from testcases
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64166

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

[Bug jit/64020] jit misses sin/cos optimizations

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64020

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #10 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Fixed with (In reply to David Malcolm from comment #7)
> (In reply to dmalcolm from comment #5)
> > (In reply to dmalcolm from comment #4)
> > > Am testing a candidate patch for this
> > 
> > Patch posted as:
> > https://gcc.gnu.org/ml/gcc-patches/2014-11/msg03229.html
> 
> Committed as r218240.
> 
> Should be fixed as of that commit, but the testcase does not yet
> automatically verify the fix.  I'll keep this open until it does (which will
> require implementing a good way to do that).

Verification of the fix implemented within test-functions.c in r218521.