This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]
Other format:	[Raw text]

Re: [PATCH 4/4] OpenMP 4.0 offloading to Intel MIC: non-fallback testing

From: Thomas Schwinge <thomas at codesourcery dot com>
To: Ilya Verbin <iverbin at gmail dot com>, Jakub Jelinek <jakub at redhat dot com>
Cc: <gcc-patches at gcc dot gnu dot org>, Kirill Yukhin <kirill dot yukhin at gmail dot com>, Andrey Turetskiy <andrey dot turetskiy at gmail dot com>
Date: Wed, 17 Dec 2014 23:48:01 +0100
Subject: Re: [PATCH 4/4] OpenMP 4.0 offloading to Intel MIC: non-fallback testing
Authentication-results: sourceware.org; auth=none
References: <20141021171323 dot GA47586 at msticlxl57 dot ims dot intel dot com> <20141030114001 dot GA34120 at msticlxl57 dot ims dot intel dot com> <20141106175540 dot GN5026 at tucnak dot redhat dot com> <20141110143430 dot GB50497 at msticlxl57 dot ims dot intel dot com>

Hi!

On Mon, 10 Nov 2014 17:34:30 +0300, Ilya Verbin <iverbin@gmail.com> wrote:
> On 06 Nov 18:55, Jakub Jelinek wrote:
> > Looks mostly good, but:
> > 
> > > +# We need more things in site.exp, but automake completely controls the
> > > +# creation of that file; there's no way to append to it without messing up
> > > +# the dependancy chains.  So we overrule automake.  This rule is exactly
> > > +# what it would have generated, plus our own additions.
> > > +site.exp: Makefile
> > > +	@echo 'Making a new site.exp file...'
> > > +	@echo '## these variables are automatically generated by make ##' >site.tmp
> > > +	@echo '# Do not edit here.  If you wish to override these values' >>site.tmp
> > > +	@echo '# edit the last section' >>site.tmp
> > > +	@echo 'set srcdir $(srcdir)' >>site.tmp
> > > +[...]
> > > +	@echo 'set target_triplet $(target_triplet)' >>site.tmp
> > > +	@echo 'set offload_targets "$(offload_targets)"' >>site.tmp
> > > +	@echo 'set offload_additional_options "$(offload_additional_options)"' >>site.tmp
> > > +	@echo 'set offload_additional_lib_paths "$(offload_additional_lib_paths)"' >>site.tmp
> > > +	@echo '## All variables above are generated by configure. Do Not Edit ##' >>site.tmp
> > > +	@test ! -f site.exp || \
> > > +	  sed '1,/^## All variables above are.*##/ d' site.exp >> site.tmp
> > > +	@-rm -f site.bak
> > > +	@test ! -f site.exp || mv site.exp site.bak
> > > +	@mv site.tmp site.exp
> > 
> > I don't like this, that is too fragile.  If automake is changed, we'll
> > forget to update this.

(The same problem exists elsewhere in GCC.  But I certainly do agree that
it's ugly.)

> > If all you are about are the 3 additional variables, can't you instead
> > put them into env vars and query them in the tcl code using getenv?
> > Or append them into AM_RUNTESTFLAGS ?
> > AM_RUNTESTFLAGS += @something@
> 
> Done, I put them into env vars.

> --- a/libgomp/testsuite/Makefile.am
> +++ b/libgomp/testsuite/Makefile.am
> @@ -11,3 +11,8 @@ EXPECT = $(shell if test -f $(top_builddir)/../expect/expect; then \
>  _RUNTEST = $(shell if test -f $(top_srcdir)/../dejagnu/runtest; then \
>  	     echo $(top_srcdir)/../dejagnu/runtest; else echo runtest; fi)
>  RUNTEST = "$(_RUNTEST) $(AM_RUNTESTFLAGS)"
> +
> +# Used for support non-fallback offloading.
> +export OFFLOAD_TARGETS = $(offload_targets)
> +export OFFLOAD_ADDITIONAL_OPTIONS = $(offload_additional_options)
> +export OFFLOAD_ADDITIONAL_LIB_PATHS = $(offload_additional_lib_paths)

> --- a/libgomp/testsuite/lib/libgomp.exp
> +++ b/libgomp/testsuite/lib/libgomp.exp
> @@ -107,6 +107,25 @@ proc libgomp_init { args } {
>      # Compute what needs to be put into LD_LIBRARY_PATH
>      set always_ld_library_path ".:${blddir}/.libs"
>  
> +    # Get offload-related variables from environment (exported by Makefile)
> +    set offload_targets [getenv OFFLOAD_TARGETS]
> +    set offload_additional_options [getenv OFFLOAD_ADDITIONAL_OPTIONS]
> +    set offload_additional_lib_paths [getenv OFFLOAD_ADDITIONAL_LIB_PATHS]
> +[...]

I have another suggestion: on gomp-4_0-branch, we had already started
using a libgomp-test-support.exp file for a similar purpose.  I now
changed your code on gomp-4_0-branch in r218845 as follows (though, not
very much tested).  The advantage here is that people who are not using
GCC build-tree testing, but are directly invoking runtest, then don't
have to set various environment variables, but instead just have to copy
this one libgomp-test-support.exp file from the build tree to the testing
tree.  Does this make sense?

commit de01639bf47e29a20959771e587fb6f30c372a45
Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Wed Dec 17 22:45:05 2014 +0000

    libgomp: Route offloading data through the libgomp-test-support.exp file.
    
    	libgomp/
    	* testsuite/Makefile.am: Don't export OFFLOAD_TARGETS,
    	OFFLOAD_ADDITIONAL_OPTIONS, and OFFLOAD_ADDITIONAL_LIB_PATHS...
    	* testsuite/libgomp-test-support.exp.in: ..., and instead set
    	offload_targets, offload_additional_options, and
    	offload_additional_lib_paths here.  Update all users.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gomp-4_0-branch@218845 138bc75d-0d04-0410-961f-82ee72b054a4
---
 libgomp/ChangeLog.gomp                        | 6 ++++++
 libgomp/testsuite/Makefile.am                 | 5 -----
 libgomp/testsuite/Makefile.in                 | 5 -----
 libgomp/testsuite/lib/libgomp.exp             | 8 +++-----
 libgomp/testsuite/libgomp-test-support.exp.in | 4 ++++
 5 files changed, 13 insertions(+), 15 deletions(-)

diff --git libgomp/ChangeLog.gomp libgomp/ChangeLog.gomp
index 1fca220..9c23c80 100644
--- libgomp/ChangeLog.gomp
+++ libgomp/ChangeLog.gomp
@@ -1,5 +1,11 @@
 2014-12-17  Thomas Schwinge  <thomas@codesourcery.com>
 
+	* testsuite/Makefile.am: Don't export OFFLOAD_TARGETS,
+	OFFLOAD_ADDITIONAL_OPTIONS, and OFFLOAD_ADDITIONAL_LIB_PATHS...
+	* testsuite/libgomp-test-support.exp.in: ..., and instead set
+	offload_targets, offload_additional_options, and
+	offload_additional_lib_paths here.  Update all users.
+
 	* testsuite/libgomp.oacc-c++/c++.exp
 	(check_effective_target_oacc_c): Remove, and ...
 	* testsuite/libgomp.oacc-c/c.exp (check_effective_target_oacc_c):
diff --git libgomp/testsuite/Makefile.am libgomp/testsuite/Makefile.am
index 9cc103a..561b7e2 100644
--- libgomp/testsuite/Makefile.am
+++ libgomp/testsuite/Makefile.am
@@ -11,8 +11,3 @@ EXPECT = $(shell if test -f $(top_builddir)/../expect/expect; then \
 _RUNTEST = $(shell if test -f $(top_srcdir)/../dejagnu/runtest; then \
 	     echo $(top_srcdir)/../dejagnu/runtest; else echo runtest; fi)
 RUNTEST = "$(_RUNTEST) $(AM_RUNTESTFLAGS)"
-
-# Used for support non-fallback offloading.
-export OFFLOAD_TARGETS = $(offload_targets)
-export OFFLOAD_ADDITIONAL_OPTIONS = $(offload_additional_options)
-export OFFLOAD_ADDITIONAL_LIB_PATHS = $(offload_additional_lib_paths)
diff --git libgomp/testsuite/Makefile.in libgomp/testsuite/Makefile.in
index 78b6351..016d0d4 100644
--- libgomp/testsuite/Makefile.in
+++ libgomp/testsuite/Makefile.in
@@ -421,11 +421,6 @@ uninstall-am:
 	uninstall uninstall-am
 
 
-# Used for support non-fallback offloading.
-export OFFLOAD_TARGETS = $(offload_targets)
-export OFFLOAD_ADDITIONAL_OPTIONS = $(offload_additional_options)
-export OFFLOAD_ADDITIONAL_LIB_PATHS = $(offload_additional_lib_paths)
-
 # Tell versions [3.59,3.63) of GNU make to not export all variables.
 # Otherwise a system limit (for SysV at least) may be exceeded.
 .NOEXPORT:
diff --git libgomp/testsuite/lib/libgomp.exp libgomp/testsuite/lib/libgomp.exp
index 81545db..6600c25 100644
--- libgomp/testsuite/lib/libgomp.exp
+++ libgomp/testsuite/lib/libgomp.exp
@@ -111,13 +111,9 @@ proc libgomp_init { args } {
     # Compute what needs to be put into LD_LIBRARY_PATH
     set always_ld_library_path ".:${blddir}/.libs"
 
-    # Get offload-related variables from environment (exported by Makefile)
-    set offload_targets [getenv OFFLOAD_TARGETS]
-    set offload_additional_options [getenv OFFLOAD_ADDITIONAL_OPTIONS]
-    set offload_additional_lib_paths [getenv OFFLOAD_ADDITIONAL_LIB_PATHS]
-
     # Add liboffloadmic build directory in LD_LIBRARY_PATH to support
     # non-fallback testing for Intel MIC targets
+    global offload_targets
     if { [string match "*-intelmic-*" $offload_targets]
 	|| [string match "*-intelmicemul-*" $offload_targets] } {
 	append always_ld_library_path ":${blddir}/../liboffloadmic/.libs"
@@ -126,6 +122,7 @@ proc libgomp_init { args } {
 	append always_ld_library_path ":${blddir}/../libstdc++-v3/src/.libs"
     }
 
+    global offload_additional_lib_paths
     if { $offload_additional_lib_paths != "" } {
 	append always_ld_library_path "${offload_additional_lib_paths}"
     }
@@ -215,6 +212,7 @@ proc libgomp_init { args } {
 
     # Used for support non-fallback offloading.
     # Help GCC to find target mkoffload.
+    global offload_additional_options
     if { $offload_additional_options != "" } {
 	lappend ALWAYS_CFLAGS "additional_flags=${offload_additional_options}"
     }
diff --git libgomp/testsuite/libgomp-test-support.exp.in libgomp/testsuite/libgomp-test-support.exp.in
index dcadad7..4586fbe 100644
--- libgomp/testsuite/libgomp-test-support.exp.in
+++ libgomp/testsuite/libgomp-test-support.exp.in
@@ -1,2 +1,6 @@
 set cuda_driver_include "@CUDA_DRIVER_INCLUDE@"
 set cuda_driver_lib "@CUDA_DRIVER_LIB@"
+
+set offload_targets "@offload_targets@"
+set offload_additional_options "@offload_additional_options@"
+set offload_additional_lib_paths "@offload_additional_lib_paths@"


GrÃÃe,
 Thomas

Attachment: signature.asc
Description: PGP signature

Follow-Ups:
- Re: [PATCH 4/4] OpenMP 4.0 offloading to Intel MIC: non-fallback testing
  - From: Jakub Jelinek

Index Nav:	[Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav:	[Date Prev] [Date Next]	[Thread Prev] [Thread Next]