Re: [PATCH] Support simd function declarations via a pre-include.

[Paul Richard Thomas <paul dot richard dot thomas at gmail dot com>]

Hi All,

Forget my remark about mp_prop_design.f90. ifort -parallel means just
that and has nothing to do with vectorization.

Sorry for the noise.

Paul

On Sat, 17 Nov 2018 at 13:29, Paul Richard Thomas
<paul.richard.thomas@gmail.com> wrote:
>
> Hi All,
>
> I took a few moments away from what I really must be doing to try out
> an earlier version of the patch. There are quite a few CRs in the
> patch and the third chunk in gcc.c was rejected, although I cannot see
> why. I made a change to scanner.c to prevent the segfault that results
> from not having the pre-include file in the right directory - see
> attached.
>
> Everything works fine with the testcases and a slightly evolved
> version shows reasonable speed-ups:
> program test_overloaded_intrinsic
>   real(4) :: x4(3200), y4(3200, 10000)
>   real(8) :: x8(3200), y8(3200)
>
>    do i = 1,10000
>     y4(:,i) = cos(x4)
>     y4(:,i) = x4*y4(:,i)
>     y4(:,i) = sqrt(y4(:,i))
>   end do
>   print *, y4(1,1)
> end
>
> Disappointingly, though, one of the worst cases in
> https://www.fortran.uk/fortran-compiler-comparisons/ of a big
> difference between gfortran and ifort with vectorization,
> mp_prop_design.f90, doesn't seem to pick up any of the vectorization
> opportunities, even though it is peppered with trig functions. Should
> I be expecting this? Yes, I did add the other trig functions to the
> pre-include file :-)
>
> Best regards and thanks for taking care of this
>
> Paul
> On Fri, 16 Nov 2018 at 15:13, Martin Liška <mliska@suse.cz> wrote:
> >
> > On 11/16/18 2:49 PM, Jakub Jelinek wrote:
> > > On Fri, Nov 16, 2018 at 02:24:42PM +0100, Martin Liška wrote:
> > >> +  if (gfc_match (" (%n) attributes simd", builtin) != MATCH_YES)
> > >> +    return MATCH_ERROR;
> > >> +
> > >> +  int builtin_kind = 0;
> > >> +  if (gfc_match (" (notinbranch)") == MATCH_YES)
> > >
> > > I think you need " ( notinbranch )" here.
> > >
> > >> +    builtin_kind = -1;
> > >> +  else if (gfc_match (" (inbranch)") == MATCH_YES)
> > >> +    builtin_kind = 1;
> > >
> > > And similarly here (+ testsuite coverage for whether you can in free form
> > > insert spaces in all the spots that should be allowed).
> > > !gcc$ builtin ( sinf ) attributes simd ( notinbranch )  ! comment
> > > e.g. should be valid in free form (and fixed form too).
> > >
> > >> --- a/gcc/fortran/gfortran.h
> > >> +++ b/gcc/fortran/gfortran.h
> > >> @@ -2764,6 +2764,18 @@ bool gfc_in_match_data (void);
> > >>  match gfc_match_char_spec (gfc_typespec *);
> > >>  extern int directive_unroll;
> > >>
> > >> +/* Tuple for parsing of vectorized built-ins.  */
> > >> +struct vect_builtin_tuple
> > >> +{
> > >> +  vect_builtin_tuple (const char *n, int t): name (n), simd_type (t)
> > >
> > > gfc_vect_builtin_tuple ?
> > > + document what the simd_type is (or make it enum or whatever).
> > > One option would be enum omp_clause_code and use OMP_CLAUSE_ERROR for
> > > the case where the argument isn't specified, but I think generally
> > > gfortran.h doesn't depend on tree* stuff and wants to have its own
> > > enums etc.
> > >
> > >> +extern vec<vect_builtin_tuple> vectorized_builtins;
> > >
> > > gfc_vectorized_builtins ?
> > >
> > >> --- a/gcc/fortran/trans-intrinsic.c
> > >> +++ b/gcc/fortran/trans-intrinsic.c
> > >> @@ -597,7 +597,61 @@ define_quad_builtin (const char *name, tree type, bool is_const)
> > >>    return fndecl;
> > >>  }
> > >>
> > >> +/* Add SIMD attribute for FNDECL built-in if the built-in
> > >> +   name is in VECTORIZED_BUILTINS.  */
> > >> +#include "print-tree.h"
> > >
> > > If you need to include a header, include it at the start of the file.
> > >
> > >> +static void
> > >> +add_simd_flag_for_built_in (tree fndecl)
> > >> +{
> > >> +  if (fndecl == NULL_TREE)
> > >> +    return;
> > >> +
> > >> +  const char *name = IDENTIFIER_POINTER (DECL_NAME (fndecl));
> > >> +  for (unsigned i = 0; i < vectorized_builtins.length (); i++)
> > >> +    if (strcmp (vectorized_builtins[i].name, name) == 0)
> > >
> > > How many add_simd_flag_for_built_in calls are we expecting and how many
> > > vectorized_builtins.length ()?  If it is too much, perhaps e.g. sort
> > > the vector by name and do a binary search.  At least if it turns out to be
> > > non-trivial compile time.
> > >> +
> > >> +  vectorized_builtins.truncate (0);
> > >
> > > That is a memory leak, right?  The names are malloced.
> > > And why truncate rather than release?
> > >> +  const char *path = find_a_file (&include_prefixes, argv[1], R_OK, true);
> > >> +  if (path != NULL)
> > >> +      return concat (argv[0], path, NULL);
> > >
> > > Formatting.
> > >> --- /dev/null
> > >> +++ b/gcc/testsuite/gfortran.dg/simd-builtins-1.h
> > >> @@ -0,0 +1,4 @@
> > >> +!GCC$ builtin (sinf) attributes simd
> > >> +!GCC$ builtin (sinf) attributes simd (inbranch)
> > >> +!GCC$ builtin (sinf) attributes simd (notinbranch)
> > >> +!GCC$ builtin (cosf) attributes simd (notinbranch)
> > >
> > > Are you sure it is a good idea to have the 3 first lines for the same
> > > builtin, rather than different?
> > >
> > > It should be testsuite covered what we do in that case, but with the above
> > > you don't cover what happens e.g. with notinbranch alone, or no argument.
> > >
> > > Plus, as I said, I think you should have one *.f and one *.f90 test where
> > > you just use many of those !gcc$ builtin lines with spaces in various spots
> > > to verify it is parsed properly.
> > >
> > >       Jakub
> > >
> >
> > Hi.
> >
> > I'm sending version, I changed the container to hash_map that should provide
> > faster look up.
> >
> > I've been testing the patch right now.
> >
> > Martin
>
>
>
> --
> "If you can't explain it simply, you don't understand it well enough"
> - Albert Einstein



-- 
"If you can't explain it simply, you don't understand it well enough"
- Albert Einstein