Hi! On Fri, 26 Sep 2014 16:36:21 +0400, Ilya Verbin <iver...@gmail.com> wrote: > --- a/gcc/Makefile.in > +++ b/gcc/Makefile.in > @@ -58,6 +58,7 @@ build=@build@ > host=@host@ > target=@target@ > target_noncanonical:=@target_noncanonical@ > +real_target_noncanonical:=@real_target_noncanonical@ > > # Sed command to transform gcc to installed name. > program_transform_name := @program_transform_name@ > @@ -66,6 +67,10 @@ program_transform_name := @program_transform_name@ > # Directories used during build > # ----------------------------- > > +# Normally identical to target_noncanonical, except for compilers built > +# as accelerator targets. > +accel_dir_suffix = @accel_dir_suffix@
Doesn't that comment belong to real_target_noncanonical just above? By
default, accel_dir_suffix is empty. Probably also move accel_dir_suffix
above to where real_target_noncanonical is being set?
> --- a/configure.ac
> +++ b/configure.ac
> @@ -286,6 +286,24 @@ case ${with_newlib} in
> yes) skipdirs=`echo " ${skipdirs} " | sed -e 's/ target-newlib / /'` ;;
> esac
>
> +AC_ARG_ENABLE(as-accelerator-for,
> +[AS_HELP_STRING([--enable-as-accelerator-for=ARG],
> + [build as offload target compiler.
> + Specify offload host triple by ARG])],
> +ENABLE_AS_ACCELERATOR_FOR=$enableval,
> +ENABLE_AS_ACCELERATOR_FOR=no)
I don't see $ENABLE_AS_ACCELERATOR_FOR being used anywhere, so this can
probably be removed?
Also, given that we're addding --enable-as-accelerator-for and
--enable-offload-targets to the top-level configure, do we need really to
repeat (and thus, in the future maintain) those also in the subdirectory
configure files where the respective enable_* flags are evaulated? If my
understanding of Autoconf is correct, then the enable_* variables will be
available in the subdirectory configure files, even without repeating the
AC_ARG_ENABLE instantiations.
How is this handled for other --enable-[...] flags that are needed in
several configure files?
> --- a/gcc/configure.ac
> +++ b/gcc/configure.ac
> @@ -883,6 +887,53 @@ AC_ARG_ENABLE(languages,
> esac],
> [enable_languages=c])
>
> +AC_ARG_ENABLE(as-accelerator-for,
> +[AS_HELP_STRING([--enable-as-accelerator-for=ARG],
> + [build as offload target compiler.
> + Specify offload host triple by ARG])],
> +[
> + AC_DEFINE(ACCEL_COMPILER, 1,
> + [Define if this compiler should be built as the offload target
> compiler.])
> + enable_as_accelerator=yes
> + case "${target}" in
> + *-intelmicemul-*)
> + # In this case we expect offload compiler to be built as native, so we
> + # need to rename the driver to avoid clashes with host's drivers.
> + program_transform_name="s&^&${target}-&" ;;
> + esac
> +
> sedscript="s#${target_noncanonical}#${enable_as_accelerator_for}-accel-${target_noncanonical}#"
> + program_transform_name=`echo $program_transform_name | sed $sedscript`
> + accel_dir_suffix=/accel/${target_noncanonical}
> + real_target_noncanonical=${enable_as_accelerator_for}
> +], [enable_as_accelerator=no])
> +AC_SUBST(enable_as_accelerator)
Thus, here you should be able to remove the AC_ARG_ENABLE for
--enable-as-accelerator-for, and just do something like (untested):
if test x"$enable_as_accelerator" != x; then
AC_DEFINE([...])
[...]
fi
I'm not sure whether the AC_SUBST for enable_as_accelerator needs to be
repeated. (I think it needs to be repeated.)
> +AC_ARG_ENABLE(offload-targets,
Likewise, and also in the other */configure.ac files.
> --- a/gcc/configure.ac
> +++ b/gcc/configure.ac
> +AC_SUBST(real_target_noncanonical)
> +AC_SUBST(accel_dir_suffix)
Can't we move these two AC_SUBST invocation up to where the variables are
being defined?
> --- /dev/null
> +++ b/libgcc/ompstuff.c
> @@ -0,0 +1,80 @@
> +/* Specialized bits of code needed for the OpenMP offloading tables.
> +#define OFFLOAD_FUNC_TABLE_SECTION_NAME "__gnu_offload_funcs"
> +#define OFFLOAD_VAR_TABLE_SECTION_NAME "__gnu_offload_vars"
Here we use __gnu_offload_* names here, and...
> +void *_omp_func_table[0]
> + __attribute__ ((__used__, visibility ("hidden"),
> + section (OFFLOAD_FUNC_TABLE_SECTION_NAME))) = { };
> +void *_omp_var_table[0]
> + __attribute__ ((__used__, visibility ("hidden"),
> + section (OFFLOAD_VAR_TABLE_SECTION_NAME))) = { };
..., also use OFFLOAD_*_TABLE_*, but on the other hand use _omp_*_table.
For consistency, shouldn't these also be named _offload_*_table? Then
the »OpenMP« could be removed from the description in line 1 (just
»needed for the offloading tables«, and the file also be renamed to
offloadstuff.c or similar. I certainly do acknowledge the role that
OpenMP has played in the development of this, but yet, this
infrastructure is not tied to OpenMP.
> +void *__OPENMP_TARGET__[]
> + __attribute__ ((__visibility__ ("hidden"))) =
> +{
> + &_omp_func_table, &_omp_funcs_end,
> + &_omp_var_table, &_omp_vars_end
> +};
Also, the name __OPENMP_TARGET__ here is more opaque than it needs to be:
what about using __OFFLOADING_TABLE__ or similar. As I have argued
before in a similar context (and Jakub has generally agreed): »the
"target" tag is confusing in that "target" typically has a different
meaning in the compiler context -- while this one here is used for
implementing OpenMP's target construct, what it technically does should
be described by .gnu.offload_[...] or somthing similar. [...] Again, all
this doesn't matter to anyone who's already versed with the code, but it
does matter to people who are new, and still have to learn all these fine
details. (Of which there are many, many more. Yes, I know, life's
tough, and I'm used to that, but still.) ;-)«.
> --- a/libgcc/Makefile.in
> +++ b/libgcc/Makefile.in
> @@ -990,6 +990,14 @@ crtendS$(objext): $(srcdir)/crtstuff.c
> crtbeginT$(objext): $(srcdir)/crtstuff.c
> $(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $< -DCRT_BEGIN -DCRTSTUFFT_O
>
> +# crtompbegin and crtompend contain symbols, that mark the begin and the end
> of
> +# tables with addresses, required for OpenMP offloading.
> +crtompbegin$(objext): $(srcdir)/ompstuff.c
> + $(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $< -DCRT_BEGIN
> +
> +crtompend$(objext): $(srcdir)/ompstuff.c
> + $(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $< -DCRT_END
> +
If you agree, then these files would be renamed to
crtoffload[ing]{begin,end}$(objext).
Grüße,
Thomas
pgp1OMKdL3U64.pgp
Description: PGP signature
