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.tho...@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 <mli...@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