2016-02-22 18:13 GMT+03:00 Thomas Schwinge <tho...@codesourcery.com>:
> On Sat, 20 Feb 2016 13:54:20 +0300, Ilya Verbin <iver...@gmail.com> wrote:
>> On Fri, Feb 19, 2016 at 15:53:08 +0100, Jakub Jelinek wrote:
>> > On Wed, Feb 10, 2016 at 08:19:34PM +0300, Ilya Verbin wrote:
>> > > This patch adds crtoffload{begin,end}.o to all -fopenmp programs, if
>> > > they exist.
>> > > I couldn't think of a better solution...
>> > > Tested using the testcase from the previous mail, e.g.:
>> > >
>> > > $ gcc -DNUM=1 -c -fopenmp test.c -o obj1.o
>> > > $ gcc -DNUM=2 -c -fopenmp test.c -o obj2.o
>> > > $ gcc -DNUM=3 -c -fopenmp test.c -o obj3.o
>> > > $ gcc -DNUM=4 -c -fopenmp test.c -o obj4.o -flto
>> > > $ gcc -DNUM=5 -c -fopenmp test.c -o obj5.o
>> > > $ gcc -DNUM=6 -c -fopenmp test.c -o obj6.o -flto
>> > > $ gcc -DNUM=7 -c -fopenmp test.c -o obj7.o
>> > > $ gcc-ar -cvq libtest.a obj3.o obj4.o obj5.o
>> > > $ gcc -fopenmp main.c obj1.o obj2.o libtest.a obj6.o obj7.o
>> > >
>> > > And other combinations.
>
>> Thomas, could you please test it using nvptx
>
> It mostly ;-) works. With nvptx offloading enabled (which you don't
> have, do you?), I'm seeing one test case regress:
>
> [-PASS:-]{+FAIL:+}
> libgomp.oacc-c/../libgomp.oacc-c-c++-common/parallel-dims-2.c
> -DACC_DEVICE_TYPE_nvidia=1 -DACC_MEM_SHARED=0 (test for errors, line 9)
> [-PASS:-]{+FAIL:+}
> libgomp.oacc-c/../libgomp.oacc-c-c++-common/parallel-dims-2.c
> -DACC_DEVICE_TYPE_nvidia=1 -DACC_MEM_SHARED=0 (test for errors, line 13)
> PASS: libgomp.oacc-c/../libgomp.oacc-c-c++-common/parallel-dims-2.c
> -DACC_DEVICE_TYPE_nvidia=1 -DACC_MEM_SHARED=0 (test for excess errors)
> [-PASS:-]{+FAIL:+}
> libgomp.oacc-c/../libgomp.oacc-c-c++-common/parallel-dims-2.c
> -DACC_DEVICE_TYPE_nvidia=1 -DACC_MEM_SHARED=0 execution test
>
> (Same for C++.) That testcase, just recently added by Tom in r233237
> "Handle -fdiagnostics-color in lto", specifies 'dg-additional-options
> "-flto -fno-use-linker-plugin"'. Is that now an unsupported
> combination/configuration? (I have not yet looked in detail, but it
> appears as if the offloading compilers are no longer being run for
> -fno-use-linker-plugin.)
Yes, it's really hard to fix the "lto + non-lto objects" issue for
no-use-linker-plugin LTO path. In this patch lto-plugin prepares a
list of objects files with offloading and passes it to lto-wrapper, so
I believe we should consider offloading without lto-plugin as
unsupported. I'll update wiki when the patch will be committed.
>> including the testcase with static
>> libraries?
>
> Works in my manual testing if I work around the following issue:
>
>> --- a/gcc/config/gnu-user.h
>> +++ b/gcc/config/gnu-user.h
>> @@ -49,14 +49,16 @@ see the files COPYING3 and COPYING.RUNTIME respectively.
>> If not, see
>> %{" NO_PIE_SPEC ":crtbegin.o%s}} \
>> %{fvtable-verify=none:%s; \
>> fvtable-verify=preinit:vtv_start_preinit.o%s; \
>> - fvtable-verify=std:vtv_start.o%s}"
>> + fvtable-verify=std:vtv_start.o%s} \
>> + %{fopenacc|fopenmp:%:if-exists(crtoffloadbegin%O%s)}"
>
> (..., and similar for others.) The if-exists spec function only works
> for absolute paths (I have not researched, why?), so it won't locate the
> files for relative -Bbuild-gcc/[...] prefixes, and linking will fail:
>
> /tmp/ccGajPD4.crtoffloadtable.o:(.rodata+0x0): undefined reference to
> `__offload_func_table'
> /tmp/ccGajPD4.crtoffloadtable.o:(.rodata+0x8): undefined reference to
> `__offload_funcs_end'
> /tmp/ccGajPD4.crtoffloadtable.o:(.rodata+0x10): undefined reference to
> `__offload_var_table'
> /tmp/ccGajPD4.crtoffloadtable.o:(.rodata+0x18): undefined reference to
> `__offload_vars_end'
>
> If I use the absolute -B$PWD/build-gcc/[...], it works. (But there is no
> requirement for -B prefixes to be absolute, as far as I know.) Why not
> make it a hard error, though, if these files are missing? Can we use
> something like (untested pseudo-patch):
>
> +#ifdef ENABLE_OFFLOADING
> +# define CRTOFFLOADBEGIN "%{fopenacc|fopenmp:%:crtoffloadbegin%O%s}"
> +#else
> +# define CRTOFFLOADBEGIN ""
> +#endif
>
> @@ -49,14 +49,16 @@ see the files COPYING3 and COPYING.RUNTIME
> respectively. If not, see
> %{" NO_PIE_SPEC ":crtbegin.o%s}} \
> %{fvtable-verify=none:%s; \
> fvtable-verify=preinit:vtv_start_preinit.o%s; \
> - fvtable-verify=std:vtv_start.o%s}"
> + fvtable-verify=std:vtv_start.o%s} \
> + " CRTOFFLOADBEGIN ")}"
OK, I'll replace if-exists with ifdef ENABLE_OFFLOADING.
> I have not verified your patch's logic in detail (arcane...) ;-) so just
> two drive-by comments:
>
>> #else
>> #define GNU_USER_TARGET_STARTFILE_SPEC \
>> "%{!shared: %{pg|p|profile:gcrt1.o%s;:crt1.o%s}} \
>> crti.o%s %{static:crtbeginT.o%s;shared|pie:crtbeginS.o%s;:crtbegin.o%s} \
>> %{fvtable-verify=none:%s; \
>> fvtable-verify=preinit:vtv_start_preinit.o%s; \
>> - fvtable-verify=std:vtv_start.o%s}"
>> + fvtable-verify=std:vtv_start.o%s} \
>> + %{fopenacc|fopenmp:%:if-exists(crtoffloadbegin%O%s)}"
>> #endif
>> #undef STARTFILE_SPEC
>> #define STARTFILE_SPEC GNU_USER_TARGET_STARTFILE_SPEC
>> @@ -73,13 +75,15 @@ see the files COPYING3 and COPYING.RUNTIME respectively.
>> If not, see
>> fvtable-verify=preinit:vtv_end_preinit.o%s; \
>> fvtable-verify=std:vtv_end.o%s} \
>> %{shared:crtendS.o%s;: %{" PIE_SPEC ":crtendS.o%s} \
>> - %{" NO_PIE_SPEC ":crtend.o%s}} crtn.o%s"
>> + %{" NO_PIE_SPEC ":crtend.o%s}} crtn.o%s \
>> + %{fopenacc|fopenmp:%:if-exists(crtoffloadend%O%s)}"
>> #else
>> #define GNU_USER_TARGET_ENDFILE_SPEC \
>> "%{fvtable-verify=none:%s; \
>> fvtable-verify=preinit:vtv_end_preinit.o%s; \
>> fvtable-verify=std:vtv_end.o%s} \
>> - %{shared|pie:crtendS.o%s;:crtend.o%s} crtn.o%s"
>> + %{shared|pie:crtendS.o%s;:crtend.o%s} crtn.o%s \
>> + %{fopenacc|fopenmp:%:if-exists(crtoffloadend%O%s)}"
>> #endif
>> #undef ENDFILE_SPEC
>> #define ENDFILE_SPEC GNU_USER_TARGET_ENDFILE_SPEC
>
> I guess we currently don't have to care about offloading configurations
> not using the gnu-user.h file in which you modified the
> STARTFILE_SPEC/ENDFILE_SPEC?
I think so.
>> --- a/lto-plugin/lto-plugin.c
>> +++ b/lto-plugin/lto-plugin.c
>
>> @@ -671,16 +681,37 @@ all_symbols_read_handler (void)
>
>> + if (num_offload_files > 0)
>> {
>> + [...]
>> + struct plugin_offload_file *ofld;
>> + [...]
>> + ofld = offload_files->next;
>> + while (ofld)
>> + {
>> + fprintf (f, "%s\n", ofld->name);
>> + ofld = ofld->next;
>> + }
>
> To the casual reader, skipping the first offload_files looks like a
> off-by-one error, so I suggest you add a comment "Skip the dummy item at
> the start of the list.", or similar.
OK.
-- Ilya
