Re: [PATCH] BRIG frontend: request for a global review

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

On Thu, Jan 26, 2017 at 01:30:21PM +0200, Pekka Jääskeläinen wrote:
> diff --git a/ChangeLog b/ChangeLog
> index 9695f9d..6f4f256 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2017-01-26  Pekka J????skel??inen  <pekka.jaaskelainen@parmance.com>
> +
> +	* configure.ac: Added i[3456789]86-*-linux* as a supported env
> +	for the BRIG FE.
> +	* configure: Regenerated.
> +
>  2017-01-24  Pekka J????skel??inen <pekka@parmance.com>
>  	    Martin Jambor  <mjambor@suse.cz>
>  

It might be better if this isn't present directly in toplevel configure.
Can you add libhsail-rt/configure.tgt and handle it in toplevel configure.ac
similarly to libatomic, libcilkrts, liboffloadmic, libitm, libsanitizer,
libvtv and libmpx, i.e. like:
# Disable libhsail-rt on unsupported systems.
if test -d ${srcdir}/libhsail-rt; then
    if test x$enable_libhsail-rt = x; then
        AC_MSG_CHECKING([for libhsail-rt support])
        if (srcdir=${srcdir}/libhsail-rt; \
                . ${srcdir}/configure.tgt; \
                test -n "$UNSUPPORTED")
        then
            AC_MSG_RESULT([no])
	    unsupported_languages="$unsupported_languages brig"
	    # This implicitly disables also target-libhsail-rt as it won't
	    # get added to the build without BRIG FE.
        else
            AC_MSG_RESULT([yes])
        fi
    fi
fi
or so?

> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index f485bb3..d481b25 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,7 @@
> +2017-01-26  Pekka J????skel??inen <pekka.jaaskelainen@parmance.com>

1) there should be 2 spaces rather than just one on each side of the name,
so date two spaces name two spaces <mail@address>
2) when committing, please make sure the unicode characters in your
name are actually valid UTF-8, not sure if it is my MUA or your or something
in between, but I'm just seeing ???? and ??

> --- a/gcc/brig/ChangeLog
> +++ b/gcc/brig/ChangeLog
> @@ -1,3 +1,7 @@
> +2017-01-26  Pekka J????skel??inen <pekka.jaaskelainen@parmance.com>
> +
> +	Removed stale target-libbrig reference from config-lang.in
> +
>  2017-01-26  Jakub Jelinek  <jakub@redhat.com>
>  
>  	Update copyright years.
> diff --git a/gcc/brig/config-lang.in b/gcc/brig/config-lang.in
> index ab139b3..e390a16 100644
> --- a/gcc/brig/config-lang.in
> +++ b/gcc/brig/config-lang.in
> @@ -28,7 +28,7 @@ language="brig"
>  
>  compilers="brig1\$(exeext)"
>  
> -target_libs="target-libbrig target-libhsail-rt"
> +target_libs="target-libhsail-rt"

If you are BRIG maintainer (but not listed yet in MAINTAINERS?), you can
approve this yourself.

> --- a/gcc/builtin-types.def
> +++ b/gcc/builtin-types.def
> @@ -70,7 +70,7 @@ DEF_PRIMITIVE_TYPE (BT_UINTMAX, uintmax_type_node)
>  DEF_PRIMITIVE_TYPE (BT_INT8, signed_char_type_node)
>  DEF_PRIMITIVE_TYPE (BT_INT16, short_integer_type_node)
>  DEF_PRIMITIVE_TYPE (BT_UINT8, char_type_node)
> -DEF_PRIMITIVE_TYPE (BT_UINT16, short_unsigned_type_node)
> +DEF_PRIMITIVE_TYPE (BT_UINT16, uint16_type_node)
>  DEF_PRIMITIVE_TYPE (BT_UINT32, uint32_type_node)
>  DEF_PRIMITIVE_TYPE (BT_UINT64, uint64_type_node)
>  DEF_PRIMITIVE_TYPE (BT_WORD, (*lang_hooks.types.type_for_mode) (word_mode, 1))

This is ok for trunk.  Not sure about the INT8/INT16/UINT8 though,
1) char_type_node can be generally signed or unsigned, shouldn't that be
unsigned_char_type_node instead?
2) wonder what will happen on targets where char is 32-bit or similar.

> --- a/libhsail-rt/ChangeLog
> +++ b/libhsail-rt/ChangeLog
> @@ -1,3 +1,7 @@
> +2017-01-26  Pekka J????skel??inen <pekka.jaaskelainen@parmance.com>
> +
> +	* README: Added a proper description of what libhsail-rt is.
> +
>  2017-01-26  Jakub Jelinek  <jakub@redhat.com>
>  
>  	Update copyright years.
> diff --git a/libhsail-rt/README b/libhsail-rt/README
> index 2792253..64c2107 100644
> --- a/libhsail-rt/README
> +++ b/libhsail-rt/README
> @@ -1,4 +1,10 @@
> -Run autoconf2.64 && automake-1.11  to regenerate the buildfiles.
> -You might need to manually tweak the minor automake version number
> -in configure.ac and aclocal.m4 (search for 1.11.6) in case your
> -local 1.11 minor version doesn't match. 
> \ No newline at end of file
> +This library implements the agent-side runtime functionality required
> +to run HSA finalized programs produced by the BRIG frontend.
> +
> +The library contains both the code required to run kernels on the agent
> +and also functions implementing more complex HSAIL instructions.
> +
> +rt/workitems.c contains the runtime entry function that manages multiple
> +work-item execution using fibers or simple for-loops (in case of work groups
> +without barriers).  Otherwise, the rest of the source files mostly contain
> +functions that typically map directly to HSAIL instructions.

This looks good, but again, you should be able to commit without review if
you are gcc/brig and libhsail-rt maintainer.

	Jakub