Re: Request for testing on non-Linux targets; remove special casing of /usr/lib and /lib from the driver
The patch does not cause failures on AIX. Is it removing explicit references to /lib and /usr/lib? It seems more appropriate for GCC 15. Thanks for alerting me to the patch to test on AIX. AIX is in CFarm. Thanks David On Tue, Apr 16, 2024 at 7:49 PM Andrew Pinski (QUIC) < quic_apin...@quicinc.com> wrote: > Hi all, > The driver currently will remove "/lib" and "/usr/lib" from the library > path that gets passed to the linker because it considers them as paths that > the linker will already known to search. But this is not true for newer > linkers, mold and lld for an example don't have a default search path. > This patch removes the special casing to fix FreeBSD building where lld is > used by default and also fix riscv-linux-gnu when used in combination with > mold. > I have tested it on x86_64-linux-gnu and it works there but since the code > in the driver has been around since 1992, I request some folks to test it > on AIX, Mac OS (Darwin) and solaris where the ld is not GNU bfd ld as I > don't have access to those targets currently. > > Thanks, > Andrew Pinski >
Re: [PATCH] strub: Only unbias stack point for SPARC_STACK_BOUNDARY_HACK [PR113100]
If the fixes remove the failures on AIX, then the patch to disable the tests also can be reverted. Thanks, David On Wed, Jan 17, 2024 at 8:06 PM Alexandre Oliva wrote: > David, > > On Jan 7, 2024, "Kewen.Lin" wrote: > > > As PR113100 shows, the unbiasing introduced by r14-6737 can > > cause the scrubbing to overrun and screw some critical data > > on stack like saved toc base consequently cause segfault on > > Power. > > I suppose this problem that Kewen fixed (thanks) was what caused you to > install commit r14-6838. According to posted test results, strub worked > on AIX until Dec 20, when the fixes for sparc that broke strub on ppc > went in. > > I can't seem to find the email in which you posted the patch, and I'd > have appreciated if you'd copied me. I wouldn't have missed it for so > long if you had. Since I couldn't find that patch, I'm responding in > this thread instead. > > The r14-6838 patch is actually very very broken. Disabling strub on a > target is not a matter of changing only the testsuite. Your additions > to the tests even broke the strub-unsupported testcases, that tested > exactly the feature that enables ports to disable strub in a way that > informs users in case they attempt to use it. > > I'd thus like to revert that patch. > > Kewen's patch needs a little additional cleanup, that I'm preparing now, > to restore fully-functioning strub on sparc32. > > Please let me know in case you observe any other problems related with > strub. I'd be happy to fix them, but I can only do so once I'm aware of > them. > > In case the reversal or the upcoming cleanup has any negative impact, > please make sure you let me know. > > Thanks, > > Happy GNU Year! > > -- > Alexandre Oliva, happy hackerhttps://FSFLA.org/blogs/lxo/ >Free Software Activist GNU Toolchain Engineer > More tolerance and less prejudice are key for inclusion and diversity > Excluding neuro-others for not behaving ""normal"" is *not* inclusive >
Re: [committed] libstdc++: Implement C++23 header [PR107760]
On Sat, Dec 16, 2023 at 7:04 PM Jonathan Wakely wrote: > On Sun, 17 Dec 2023 at 00:02, Jonathan Wakely wrote: > > > > On Sat, 16 Dec 2023 at 23:06, David Edelsohn wrote: > > > > > > On Sat, Dec 16, 2023 at 4:44 PM Jakub Jelinek > wrote: > > >> > > >> On Sat, Dec 16, 2023 at 04:24:13PM -0500, David Edelsohn wrote: > > >> > AIX stdio.h defines fileno as a macro although there is a symbol in > libc. > > >> > > > >> > I think that print.cc at least needs to > > >> > > > >> > > > >> > #undef fileno > > >> > > > >> > > > >> > before the usage. > > >> > > >> Or (::fileno)(f) ? > > > > > > > > > Yes, that also avoids the error. > > > > Yup, I've just tested it. I'll push that change in the morning. > > Actually I just pushed it now. The functions in that file are only > actually used on Windows, so if they build on linux and AIX, that's > good enough. > Thanks. I had tested and was just about to push it myself. Thanks, David
Re: [committed] libstdc++: Implement C++23 header [PR107760]
On Sat, Dec 16, 2023 at 4:44 PM Jakub Jelinek wrote: > On Sat, Dec 16, 2023 at 04:24:13PM -0500, David Edelsohn wrote: > > AIX stdio.h defines fileno as a macro although there is a symbol in libc. > > > > I think that print.cc at least needs to > > > > > > #undef fileno > > > > > > before the usage. > > Or (::fileno)(f) ? > Yes, that also avoids the error. Thanks, David > > Jakub > >
Re: [committed] libstdc++: Implement C++23 header [PR107760]
Hi, Jonathan
Unfortunately this patch broke bootstrap on AIX.
In file included from /tmp/GCC/gcc/include-fixed/wchar.h:64,
from
/tmp/GCC/powerpc-ibm-aix7.2.5.0/libstdc++-v3/include/cwchar:44,
from
/tmp/GCC/powerpc-ibm-aix7.2.5.0/libstdc++-v3/include/bits/postypes.h:40,
from
/tmp/GCC/powerpc-ibm-aix7.2.5.0/libstdc++-v3/include/bits/char_traits.h:42,
from
/tmp/GCC/powerpc-ibm-aix7.2.5.0/libstdc++-v3/include/string:42,
from
/nasfarm/edelsohn/src/src/libstdc++-v3/src/c++23/print.cc:26:
/nasfarm/edelsohn/src/src/libstdc++-v3/src/c++23/print.cc: In function
'void* std::__open_terminal(FILE*)':
/nasfarm/edelsohn/src/src/libstdc++-v3/src/c++23/print.cc:78:24: error:
expected id-expression before '(' token
78 | if (int fd = ::fileno(f); fd >= 0 && ::isatty(fd))
|^~
make[6]: *** [Makefile:747: print.lo] Error 1
AIX stdio.h defines fileno as a macro although there is a symbol in libc.
I think that print.cc at least needs to
#undef fileno
before the usage.
Thanks, David
Re: [PATCH] libsupc++: try cxa_thread_atexit_impl at runtime
The error is: ld: 0711-317 ERROR: Undefined symbol: __cxa_thread_atexit_impl from the new, weak reference. Also, earlier in atexit_thread.cc, there is another definition protected by _GLIBCXX_HAVE___CXA_THREAD_ATEXIT_IMPL not utilized by the new reference. Thanks, David On Tue, Dec 5, 2023 at 11:10 AM David Edelsohn wrote: > Alex, > > This patch broke bootstrap on AIX. The stage1 compiler is not able to > build a program and stage2 configure fails. > > Thanks, David > >
Re: [PATCH] libsupc++: try cxa_thread_atexit_impl at runtime
Alex, This patch broke bootstrap on AIX. The stage1 compiler is not able to build a program and stage2 configure fails. Thanks, David
Re: [PATCH v3 0/2] Replace intl/ with out-of-tree GNU gettext
On Tue, Nov 21, 2023 at 8:51 AM Arsen Arsenović wrote: > > Arsen Arsenović writes: > > > Bruno Haible writes: > > > >> Arsen Arsenović wrote: > >>> Comparing stages 2 and 3 > >>> Bootstrap comparison failure! > >>> gettext/libasprintf/autosprintf.o differs > >>> make[2]: *** [Makefile:23435: compare] Error 1 > >> > >> You should be able to work around this by passing the additional option > >> --disable-libasprintf to gettext-runtime/configure. Nothing in GCC needs > >> libasprintf; therefore there is no need to build it. > > > > Ah, sure, that works for me too (note that the fix is to pass > > -frandom-seed=, according to Jakub, should this show up again). > > Indeed, that got a bootstrap to pass. I've also taken the opportunity > to check the problems Eric Gallager reported. The install tree seems > clean now, and the info et al targets appear to work again. David, > Eric, could you check whether the attached patch works for you in the > scenarios you ran into problems with? Make sure to fetch gettext-0.22.4 > into your trees. > > > If these work, I'll update download_prerequisites and see about posting > the patch for review. > > Thanks, have a lovely day. > I don't build in tree, but the patch seems to address the previous issues. Thanks, David
Re: [PATCH v3 0/2] Replace intl/ with out-of-tree GNU gettext
_GLOBAL__F_xxx is the EH frame data. It's using the filename with full path for the unique name, which is why it includes .._.._.. . Apparently it is adding a random number as well for uniqueness. I guess that this is the downside of building in tree, and apparently it is rebuilding gettext itself with the different stages of the compiler, so the appended random number changes. Thanks, David On Mon, Nov 20, 2023 at 4:23 PM Arsen Arsenović wrote: > > David Edelsohn writes: > > > On Sun, Nov 19, 2023 at 5:15 PM Bruno Haible wrote: > > > >> David Edelsohn wrote: > >> > --disable-threads currently does not completely disable threads. > Bruno > >> is > >> > suggesting --enable-threads=isoc that relies on mtx mutex functions in > >> libc. > >> > >> Unfortunately, as said in the other mail today, relying only on mtx_* > >> functions > >> did not drop the dependency towards libpthreads. > >> > >> So, I've made a new release gettext-0.22.4, that includes only these > >> changes: > >> > >> - AM_GNU_GETTEXT now recognizes a statically built libintl on macOS > and > >> AIX. > >> > >> - Passing --disable-threads now builds a libintl that, on AIX, does > not > >> need -lpthread. > >> > >> - Other build fixes on AIX. > >> > >> > Yes, GCC should configure the in tree gettext with --disable-threads, > but > >> > that configure option is not completely effective and does not > produce a > >> > build without threads references. > >> > >> Now it is effective. But you (Arsen) should state in the documentation > >> (gcc/doc/install.texi) that for --disable-threads to have this effect, > >> one needs gettext version 0.22.4 or newer. > >> > > > > So the question is do we want to change GCC on AIX to always link against > > pthreads so that GCC can build with default, external builds of gettext > > libintl. I don't see a path for i18n support to work for GCC on AIX > > without that unfortunate change. > > Well, if detectable by the build system for, I imagine we could avoid > pthread if gettext is built without them. With the 'private' gettext > build, we should never need threads anyway. > > P.S: Building on AIX is nearly successful. gettext-0.22.4 builds, twice > or even thrice, but ends up producing a bootstrap comparison fail: > > make[3]: Leaving directory '/home/arsen/build' > Comparing stages 2 and 3 > Bootstrap comparison failure! > gettext/libasprintf/autosprintf.o differs > make[2]: *** [Makefile:23435: compare] Error 1 > > Upon inspecting these files, I see the following diff: > > ~ 1 $ git diff <(objdump --all-headers autosprintf.o2) <(objdump > --all-headers autosprintf.o3) > diff --git a/dev/fd/63 b/dev/fd/62 > --- a/dev/fd/63 > +++ b/dev/fd/62 > ... > @@ -92,7 +92,7 @@ AUX indx 30 prmhsh 0 snhsh 0 typ 2 algn 0 clss 0 stb 0 > snstb 0 > AUX val23 prmhsh 0 snhsh 0 typ 1 algn 4 clss 1 stb 0 snstb 0 > [ 58](sec 1)(fl 0x00)(ty0)(scl 107) (nx 1) 0x0460 > _autosprintf.ro_ > AUX val 312 prmhsh 0 snhsh 0 typ 1 algn 4 clss 1 stb 0 snstb 0 > -[ 60](sec 1)(fl 0x00)(ty0)(scl 2) (nx 1) 0x0460 > _GLOBAL__F_.._.._.._gcc_gettext_gettext_runtime_libasprintf_autosprintf.cc_DFF67DD7_0xa20d51b1d7a1772f > +[ 60](sec 1)(fl 0x00)(ty0)(scl 2) (nx 1) 0x0460 > _GLOBAL__F_.._.._.._gcc_gettext_gettext_runtime_libasprintf_autosprintf.cc_DFF67DD7_0x9c04058e89d7a7a4 > AUX indx 58 prmhsh 0 snhsh 0 typ 2 algn 0 clss 1 stb 0 snstb 0 > [ 62](sec 2)(fl 0x00)(ty0)(scl 107) (nx 1) 0x05a0 > _autosprintf.rw_ > AUX val 0 prmhsh 0 snhsh 0 typ 1 algn 4 clss 5 stb 0 snstb 0 > > I am unsure what this symbol is. It does not appear in the stripped > binary. > -- > Arsen Arsenović >
Re: [PATCH v3 0/2] Replace intl/ with out-of-tree GNU gettext
On Sun, Nov 19, 2023 at 5:15 PM Bruno Haible wrote: > David Edelsohn wrote: > > --disable-threads currently does not completely disable threads. Bruno > is > > suggesting --enable-threads=isoc that relies on mtx mutex functions in > libc. > > Unfortunately, as said in the other mail today, relying only on mtx_* > functions > did not drop the dependency towards libpthreads. > > So, I've made a new release gettext-0.22.4, that includes only these > changes: > > - AM_GNU_GETTEXT now recognizes a statically built libintl on macOS and > AIX. > > - Passing --disable-threads now builds a libintl that, on AIX, does not > need -lpthread. > > - Other build fixes on AIX. > > > Yes, GCC should configure the in tree gettext with --disable-threads, but > > that configure option is not completely effective and does not produce a > > build without threads references. > > Now it is effective. But you (Arsen) should state in the documentation > (gcc/doc/install.texi) that for --disable-threads to have this effect, > one needs gettext version 0.22.4 or newer. > So the question is do we want to change GCC on AIX to always link against pthreads so that GCC can build with default, external builds of gettext libintl. I don't see a path for i18n support to work for GCC on AIX without that unfortunate change. Thanks, David
Re: [PATCH v3 0/2] Replace intl/ with out-of-tree GNU gettext
On Fri, Nov 17, 2023 at 10:17 AM Arsen Arsenović wrote:
>
> David Edelsohn writes:
>
> > On Fri, Nov 17, 2023 at 3:46 AM Arsen Arsenović wrote:
> >
> >>
> >> David Edelsohn writes:
> >>
> >> > On Thu, Nov 16, 2023 at 5:52 PM Arsen Arsenović
> wrote:
> >> >
> >> > [snip]
> >> >> Sure, but my patch does insert --disable-shared:
> >> >>
> >> >> --8<---cut here---start->8---
> >> >> host_modules= { module= gettext; bootstrap=true; no_install=true;
> >> >> module_srcdir= "gettext/gettext-runtime";
> >> >> // We always build gettext with pic, because some
> >> packages
> >> >> (e.g. gdbserver)
> >> >> // need it in some configuratons, which is determined
> >> via
> >> >> nontrivial tests.
> >> >> // Always enabling pic seems to make sense for
> something
> >> >> tied to
> >> >> // user-facing output.
> >> >> extra_configure_flags='--disable-shared
> --disable-java
> >> >> --disable-csharp --with-pic';
> >> >> lib_path=intl/.libs; };
> >> >> --8<---cut here---end--->8---
> >> >>
> >> >> ... and it is applied:
> >> >>
> >> >> --8<---cut here---start->8---
> >> >> -bash-5.1$ ./config.status --config
> >> >> --srcdir=../../gcc/gettext/gettext-runtime
> --cache-file=./config.cache
> >> >> --disable-werror --with-gmp=/opt/cfarm
> >> >> --with-libiconv-prefix=/opt/cfarm --disable-libstdcxx-pch
> >> >> --with-included-gettext --program-transform-name=s,y,y,
> >> >> --disable-option-checking --build=powerpc-ibm-aix7.3.1.0
> >> >> --host=powerpc-ibm-aix7.3.1.0 --target=powerpc-ibm-aix7.3.1.0
> >> >> --disable-intermodule --enable-checking=yes,types,extra
> >> >> --disable-coverage --enable-languages=c,c++
> >> >> --disable-build-format-warnings --disable-shared --disable-java
> >> >> --disable-csharp --with-pic build_alias=powerpc-ibm-aix7.3.1.0
> >> >> host_alias=powerpc-ibm-aix7.3.1.0
> target_alias=powerpc-ibm-aix7.3.1.0
> >> >> CC=gcc CFLAGS=-g 'LDFLAGS=-static-libstdc++ -static-libgcc
> >> >> -Wl,-bbigtoc' 'CXX=g++ -std=c++11' CXXFLAGS=-g
> >> >> --8<---cut here---end--->8---
> >> >>
> >> >> I'm unsure how to tell what the produced binaries are w.r.t static or
> >> >> shared, but I only see .o files inside intl/.libs/libintl.a, while I
> see
> >> >> a .so.1 in (e.g.) /lib/libz.a, hinting at it not being shared (?)
> >> >>
> >> >
> >> > An AIX shared library created by libtool will look like
> >> > libfoo.a[libfoo.so.N], where N is the package major version number.
> >> > Normally with one file.
> >>
> >> > An AIX static library will look like libfoo.a[a.o, b.o, c.o]
> >> > with multiple object files.
> >> >
> >> > An AIX archive can contain a combination of shared objects and
> >> > normal object files.
> >> >
> >> > AIX normally uses the convention shr.o or shr_64.o for the name
> >> > of the shared object file. Hint, hint, an AIX archive can contain
> >> > both 32 bit and 64 bit object files or shared objects.
> >> >
> >> > I don't know why the gettext build system would create
> >> > /home/arsen/build/./gettext/intl/.libs/libintl.a(libintl.so.8)
> >> > if --disable-shared was requested. That clearly is using the
> >> > naming of a libtool AIX shared object and failing due to
> >> > the missing shared object. Although in this case, the problem
> >> > seems to be the shared library load path. AIX uses LIBPATH,
> >> > not LD_LIBRARY_PATH.
> >>
> >> It doesn't create libintl.a with a libintl.so.8 inside of it. The
> >> libintl.a contains a bunch of objects, as I'd expect of a static
> >> library:
> >>
> >> --8<---cut here---start->8---
> >> -bash-5.1$ ar -t gettext/intl/.libs/libintl.a | grep libintl
> >> -bash-5.1$ ar -t
Re: [PATCH v3 0/2] Replace intl/ with out-of-tree GNU gettext
On Fri, Nov 17, 2023 at 3:46 AM Arsen Arsenović wrote:
>
> David Edelsohn writes:
>
> > On Thu, Nov 16, 2023 at 5:52 PM Arsen Arsenović wrote:
> >
> > [snip]
> >> Sure, but my patch does insert --disable-shared:
> >>
> >> --8<---cut here---start->8---
> >> host_modules= { module= gettext; bootstrap=true; no_install=true;
> >> module_srcdir= "gettext/gettext-runtime";
> >> // We always build gettext with pic, because some
> packages
> >> (e.g. gdbserver)
> >> // need it in some configuratons, which is determined
> via
> >> nontrivial tests.
> >> // Always enabling pic seems to make sense for something
> >> tied to
> >> // user-facing output.
> >> extra_configure_flags='--disable-shared --disable-java
> >> --disable-csharp --with-pic';
> >> lib_path=intl/.libs; };
> >> --8<---cut here---end--->8---
> >>
> >> ... and it is applied:
> >>
> >> --8<---cut here---start->8---
> >> -bash-5.1$ ./config.status --config
> >> --srcdir=../../gcc/gettext/gettext-runtime --cache-file=./config.cache
> >> --disable-werror --with-gmp=/opt/cfarm
> >> --with-libiconv-prefix=/opt/cfarm --disable-libstdcxx-pch
> >> --with-included-gettext --program-transform-name=s,y,y,
> >> --disable-option-checking --build=powerpc-ibm-aix7.3.1.0
> >> --host=powerpc-ibm-aix7.3.1.0 --target=powerpc-ibm-aix7.3.1.0
> >> --disable-intermodule --enable-checking=yes,types,extra
> >> --disable-coverage --enable-languages=c,c++
> >> --disable-build-format-warnings --disable-shared --disable-java
> >> --disable-csharp --with-pic build_alias=powerpc-ibm-aix7.3.1.0
> >> host_alias=powerpc-ibm-aix7.3.1.0 target_alias=powerpc-ibm-aix7.3.1.0
> >> CC=gcc CFLAGS=-g 'LDFLAGS=-static-libstdc++ -static-libgcc
> >> -Wl,-bbigtoc' 'CXX=g++ -std=c++11' CXXFLAGS=-g
> >> --8<---cut here---end--->8---
> >>
> >> I'm unsure how to tell what the produced binaries are w.r.t static or
> >> shared, but I only see .o files inside intl/.libs/libintl.a, while I see
> >> a .so.1 in (e.g.) /lib/libz.a, hinting at it not being shared (?)
> >>
> >
> > An AIX shared library created by libtool will look like
> > libfoo.a[libfoo.so.N], where N is the package major version number.
> > Normally with one file.
>
> > An AIX static library will look like libfoo.a[a.o, b.o, c.o]
> > with multiple object files.
> >
> > An AIX archive can contain a combination of shared objects and
> > normal object files.
> >
> > AIX normally uses the convention shr.o or shr_64.o for the name
> > of the shared object file. Hint, hint, an AIX archive can contain
> > both 32 bit and 64 bit object files or shared objects.
> >
> > I don't know why the gettext build system would create
> > /home/arsen/build/./gettext/intl/.libs/libintl.a(libintl.so.8)
> > if --disable-shared was requested. That clearly is using the
> > naming of a libtool AIX shared object and failing due to
> > the missing shared object. Although in this case, the problem
> > seems to be the shared library load path. AIX uses LIBPATH,
> > not LD_LIBRARY_PATH.
>
> It doesn't create libintl.a with a libintl.so.8 inside of it. The
> libintl.a contains a bunch of objects, as I'd expect of a static
> library:
>
> --8<---cut here---start->8---
> -bash-5.1$ ar -t gettext/intl/.libs/libintl.a | grep libintl
> -bash-5.1$ ar -t gettext/intl/.libs/libintl.a
> bindtextdom.o
> dcgettext.o
> ...
> --8<---cut here---end--->8---
>
>
> > Also, for me, the out of tree path was
> >
> > gettext/gettext-runtime/intl/.libs
> >
> > Is your search path missing a level?
>
> No, the above is generated by the GCC build system and builds
> gettext-runtime directly (per Brunos recommendation a while ago) as it
> is replacing intl/ of similar functionality.
>
> I'm currently building GCC with libintl with the threads hack you
> mentioned applied (as I got undefined references to the pthread
> functions you discovered). I suspect that, bar this issue (which, IIUC,
> Bruno will fix in a new release?) the patch above will fix the issues
> you've encountered on AIX (note that if you want to use gettext in-tree,
> you'd still have to fetch gettext into the tree).
>
> Maybe we should provide a download-prerequisite-y script that skips
> everything but GNU gettext, to retain same behavior?
>
> Have a lovely day.
>
I'm concerned that the gettext fixes are working around AIX support for
libpthread.a as opposed to making --disable-threads function.
--enabled-threads=isoc use of mtx_* is a workaround, but it's still not
allowing users to truly disable threads.
Thanks, David
Re: building GNU gettext on AIX
On Thu, Nov 16, 2023 at 7:07 PM Bruno Haible wrote: > David Edelsohn wrote: > > > ibm-clang links against libpthread.a as well: > > > $ ldd /opt/IBM/openxlC/17.1.1/bin/.ibm-clang.orig > > > /opt/IBM/openxlC/17.1.1/bin/.ibm-clang.orig needs: > > > /usr/lib/libpthreads.a(shr_xpg5_64.o) > > > /usr/opt/zlibNX/lib/libz.a(libz.so.1) > > > /usr/lib/libcurses.a(shr42_64.o) > > > /usr/lib/libiconv.a(shr4_64.o) > > > /usr/lib/libc++.a(shr_64.o) > > > /usr/lib/libc++abi.a(libc++abi.so.1) > > > /usr/lib/libc.a(shr_64.o) > > > /usr/lib/libpthreads.a(_shr_xpg5_64.o) > > > /usr/lib/libc++.a(libc++.so.1) > > > /usr/lib/libunwind.a(libunwind.so.1) > > > /usr/lib/libc.a(_shr_64.o) > > > /unix > > > /usr/lib/libcrypt.a(shr_64.o) > > > > > > > I have asked the IBM Clang team why ibm-clang depends on libpthreads. > > The reason is that > - For a library, it is a normal expectation nowadays that it is > multithread-safe. > - Making a library multithread-safe (without major hacks) means to do > locking or to call pthread_once / call_once in some places. > - The ISO C 11 threading functions in libc have some drawbacks compared > to the pthread functions. [1] So most developer prefer to rely on the > POSIX threads API. > - Since AIX does not have the POSIX mutex functions in libc and does not > support weak symbols like in ELF, this means a dependency to > pthread_mutex_lock or pthread_once. > - Accordingly, in the list of libraries above, 3 libraries need pthread* > symbols: > > $ nm -X 64 /usr/lib/libc++abi.a | grep ' U ' | grep pthread_mutex > pthread_mutex_lock U - > pthread_mutex_unlock U - > $ nm -X 64 /usr/lib/libc++.a | grep ' U ' | grep pthread_mutex > pthread_mutex_destroy U - > pthread_mutex_init U - > pthread_mutex_lock U - > pthread_mutex_trylock U - > pthread_mutex_unlock U - > pthread_mutexattr_destroy U - > pthread_mutexattr_init U - > pthread_mutexattr_settype U - > $ nm -X 64 /usr/opt/zlibNX/lib/libz.a | grep ' U ' | grep pthread_mutex > pthread_mutex_destroy U - > pthread_mutex_init U - > pthread_mutex_lock U - > pthread_mutex_unlock U - > There are ibm_clang and ibm_clang_r (previous xlc and xlc_r) to compile with and without thread safe. If IBM Clang team chose to only provide a thread safe version of libc++, okay, but that doesn't seem like a fundamental requirement. zlibNX is another can of worms. David
Re: [PATCH v3 0/2] Replace intl/ with out-of-tree GNU gettext
On Thu, Nov 16, 2023 at 5:52 PM Arsen Arsenović wrote:
>
> David Edelsohn writes:
>
> > On Thu, Nov 16, 2023 at 5:22 PM Arsen Arsenović wrote:
> >
> >>
> >> David Edelsohn writes:
> >>
> >> > Don't build with the dependent libraries in tree. Don't build the
> >> > dependent libraries as shared libraries. The libraries are already
> built
> >> > and in /opt/cfarm, as mentioned in the Compile Farm wiki.
> >> >
> >> > AIX is not Solaris and not Linux. It doesn't use ELF. AIX shared
> >> > libraries *ARE* shared object files in archives. Shared object
> >> versioning
> >> > is handled by multiple objects in the same archive.
> >>
> >> Hmm, I see. I removed all the deps but gettext from the tree.
> >>
> >> This leaves gettext-runtime fulfilling the previous role of intl/.
> >>
> >> However, I'm confused about how this worked before, in that case, since,
> >> IIRC, intl also produced libraries and was also put into host exports.
> >>
> >> Leaving gettext in tree produces:
> >>
> >> Could not load program gawk:
> >> Dependent module
> >> /home/arsen/build/./gettext/intl/.libs/libintl.a(libintl.so.8) could
> not be
> >> loaded.
> >> Member libintl.so.8 is not found in archive
> >>
> >> I'll try to see why intl/ didn't cause the same issue soon.
> >>
> >> Thanks, have a lovely evening.
> >>
> >
> > The previous version of "intl" was built as a static library. Configure
> in
> > the older package had the option --enable-host-shared,
> > which I did not use. Based on the failure message, the in-tree gettext
> > seems to be built as a shared library. If you explicitly
> > pass --disable-shared to the in-tree configure, you may get farther. I'm
> > currently using --disable-shared --disable-threads.
> > As we have discussed, the current gettext will retain some references to
> > pthreads despite the configure option.
>
> Sure, but my patch does insert --disable-shared:
>
> --8<---cut here---start->8---
> host_modules= { module= gettext; bootstrap=true; no_install=true;
> module_srcdir= "gettext/gettext-runtime";
> // We always build gettext with pic, because some packages
> (e.g. gdbserver)
> // need it in some configuratons, which is determined via
> nontrivial tests.
> // Always enabling pic seems to make sense for something
> tied to
> // user-facing output.
> extra_configure_flags='--disable-shared --disable-java
> --disable-csharp --with-pic';
> lib_path=intl/.libs; };
> --8<---cut here---end--->8---
>
> ... and it is applied:
>
> --8<---cut here---start->8---
> -bash-5.1$ ./config.status --config
> --srcdir=../../gcc/gettext/gettext-runtime --cache-file=./config.cache
> --disable-werror --with-gmp=/opt/cfarm
> --with-libiconv-prefix=/opt/cfarm --disable-libstdcxx-pch
> --with-included-gettext --program-transform-name=s,y,y,
> --disable-option-checking --build=powerpc-ibm-aix7.3.1.0
> --host=powerpc-ibm-aix7.3.1.0 --target=powerpc-ibm-aix7.3.1.0
> --disable-intermodule --enable-checking=yes,types,extra
> --disable-coverage --enable-languages=c,c++
> --disable-build-format-warnings --disable-shared --disable-java
> --disable-csharp --with-pic build_alias=powerpc-ibm-aix7.3.1.0
> host_alias=powerpc-ibm-aix7.3.1.0 target_alias=powerpc-ibm-aix7.3.1.0
> CC=gcc CFLAGS=-g 'LDFLAGS=-static-libstdc++ -static-libgcc
> -Wl,-bbigtoc' 'CXX=g++ -std=c++11' CXXFLAGS=-g
> --8<---cut here---end--->8---
>
> I'm unsure how to tell what the produced binaries are w.r.t static or
> shared, but I only see .o files inside intl/.libs/libintl.a, while I see
> a .so.1 in (e.g.) /lib/libz.a, hinting at it not being shared (?)
>
An AIX shared library created by libtool will look like
libfoo.a[libfoo.so.N], where N is the package major version number.
Normally with one file.
An AIX static library will look like libfoo.a[a.o, b.o, c.o]
with multiple object files.
An AIX archive can contain a combination of shared objects and
normal object files.
AIX normally uses the convention shr.o or shr_64.o for the name
of the shared object file. Hint, hint, an AIX archive can contain
both 32 bit and 64 bit object files or shared objects.
I don't know why the gettext build system would create
/home/
Re: building GNU gettext on AIX
On Thu, Nov 16, 2023 at 5:47 PM Bruno Haible wrote: > Hi David, > > > the default, distributed libintl library will not allow GCC to be built > > with NLS enabled. > > The problem is this configure test from gettext.m4 > > checking for GNU gettext in libintl... no > > It should say > > checking for GNU gettext in libintl... yes > > I reproduce it with simple hello-world package, outside GCC. > > It tests whether a program that uses gettext() can be linked with > -lintl -liconv > But now, on AIX, it needs to test whether such a program can be linked with > -lintl -liconv -lpthread > > > Were you suggesting that --enable-threads=isoc would work now or that it > > would require further changes for a future release? > > It requires a change, effectively to do as if HAVE_PTHREAD_API is undefined > if --enable-threads=isoc was provided. > > I can prepare a new gettext release that has both issues fixed: > - gettext.m4 that fixes the configure test and sets the variable LIBINTL > to "-Lsome/libdir -lintl -liconv -lpthread", > - mbrtowc.o and setlocale*.o that use mtx_* locks instead of pthread_* > mutexes when requested. > > But you then need to make up your mind w.r.t. what I wrote in the earlier > mail. > > * GCC can pass --enable-threads=isoc, to avoid the libpthread dependency > on AIX ≥ 7.2. > I have reached out to the AIX Open Source Tools team for their perspective. Normally GCC and other FOSS packages have not based their support for OS versions on official vendor support lifecycles, within reason. In fact, many users have appreciated longer duration support. > > * Or GCC can (continue to?) use the variable LIBINTL. This will work on > AIX 7.1 as well but the programs will then be linked against > libpthread. > One additional library. > $ ldd gcc > /opt/freeware/bin/gcc needs: > /usr/lib/libc.a(shr.o) > /opt/freeware/lib/libiconv.a(libiconv.so.2) > /usr/lib/libc.a(_shr.o) > /unix > /usr/lib/libcrypt.a(shr.o) > /opt/freeware/lib/libgcc_s.a(shr.o) > libpthread.a will be added to this list. > My builds of GCC only rely on AIX libc. All other libraries are statically linked. > > ibm-clang links against libpthread.a as well: > $ ldd /opt/IBM/openxlC/17.1.1/bin/.ibm-clang.orig > /opt/IBM/openxlC/17.1.1/bin/.ibm-clang.orig needs: > /usr/lib/libpthreads.a(shr_xpg5_64.o) > /usr/opt/zlibNX/lib/libz.a(libz.so.1) > /usr/lib/libcurses.a(shr42_64.o) > /usr/lib/libiconv.a(shr4_64.o) > /usr/lib/libc++.a(shr_64.o) > /usr/lib/libc++abi.a(libc++abi.so.1) > /usr/lib/libc.a(shr_64.o) > /usr/lib/libpthreads.a(_shr_xpg5_64.o) > /usr/lib/libc++.a(libc++.so.1) > /usr/lib/libunwind.a(libunwind.so.1) > /usr/lib/libc.a(_shr_64.o) > /unix > /usr/lib/libcrypt.a(shr_64.o) > I have asked the IBM Clang team why ibm-clang depends on libpthreads. One option is to add -lpthreads to the link line, even if the tools are not built thread-safe. Only the intl support would invoke the extraneous overhead. The downside is that other pthreads dependencies could sneak in. Thanks, David > > Bruno > > > >
Re: [PATCH v3 0/2] Replace intl/ with out-of-tree GNU gettext
On Thu, Nov 16, 2023 at 5:22 PM Arsen Arsenović wrote: > > David Edelsohn writes: > > > Don't build with the dependent libraries in tree. Don't build the > > dependent libraries as shared libraries. The libraries are already built > > and in /opt/cfarm, as mentioned in the Compile Farm wiki. > > > > AIX is not Solaris and not Linux. It doesn't use ELF. AIX shared > > libraries *ARE* shared object files in archives. Shared object > versioning > > is handled by multiple objects in the same archive. > > Hmm, I see. I removed all the deps but gettext from the tree. > > This leaves gettext-runtime fulfilling the previous role of intl/. > > However, I'm confused about how this worked before, in that case, since, > IIRC, intl also produced libraries and was also put into host exports. > > Leaving gettext in tree produces: > > Could not load program gawk: > Dependent module > /home/arsen/build/./gettext/intl/.libs/libintl.a(libintl.so.8) could not be > loaded. > Member libintl.so.8 is not found in archive > > I'll try to see why intl/ didn't cause the same issue soon. > > Thanks, have a lovely evening. > The previous version of "intl" was built as a static library. Configure in the older package had the option --enable-host-shared, which I did not use. Based on the failure message, the in-tree gettext seems to be built as a shared library. If you explicitly pass --disable-shared to the in-tree configure, you may get farther. I'm currently using --disable-shared --disable-threads. As we have discussed, the current gettext will retain some references to pthreads despite the configure option. Thanks, David > > > Thanks, David > > > > > > > > On Thu, Nov 16, 2023 at 4:15 PM Arsen Arsenović wrote: > > > >> > >> Arsen Arsenović writes: > >> > >> > [[PGP Signed Part:Good signature from 52C294301EA2C493 Arsen Arsenović > >> (Gentoo Developer UID) (trust ultimate) created at > >> 2023-11-16T19:47:16+0100 using EDDSA]] > >> > > >> > David Edelsohn writes: > >> > > >> >> On Wed, Nov 15, 2023 at 9:22 AM Arsen Arsenović > >> wrote: > >> >> > >> >>> > >> >>> David Edelsohn writes: > >> >>> > >> >>> > GCC had been working on AIX with NLS, using > >> "--with-included-gettext". > >> >>> > --disable-nls gets past the breakage, but GCC does not build for > me > >> on > >> >>> AIX > >> >>> > with NLS enabled. > >> >>> > >> >>> That should still work with gettext 0.22+ extracted in-tree (it > should > >> >>> be fetched by download_prerequisites). > >> >>> > >> >>> > A change in dependencies for GCC should have been announced and > more > >> >>> widely > >> >>> > socialized in the GCC development mailing list, not just GCC > patches > >> >>> > mailing list. > >> >>> > > >> >>> > I have tried both the AIX Open Source libiconv and libgettext > >> package, > >> >>> and > >> >>> > the ones that I previously built. Both fail because GCC configure > >> >>> decides > >> >>> > to disable NLS, despite being requested, while libcpp is > satisfied, > >> so > >> >>> > tools in the gcc subdirectory don't link against libiconv and the > >> build > >> >>> > fails. With the included gettext, I was able to rely on a > >> >>> self-consistent > >> >>> > solution. > >> >>> > >> >>> That is interesting. They should be using the same checks. I've > >> >>> checked trunk and regenerated files on it, and saw no significant > diff > >> >>> (some whitespace changes only). Could you post the config.log of > both? > >> >>> > >> >>> I've never used AIX. Can I reproduce this on one of the cfarm > machines > >> >>> to poke around? I've tried cfarm119, but that one lacked git, and I > >> >>> haven't poked around much further due to time constraints. > >> >>> > >> >> > >> >> The AIX system in the Compile Farm has a complete complement of Open > >> Source > >> >> software installed. > >> >> > >> >> Please ensure that /opt/freeware/bin
Re: building GNU gettext on AIX
On Thu, Nov 16, 2023 at 1:52 PM Bruno Haible wrote: > David Edelsohn wrote: > > I manually commented out HAVE_PTHREAD_API from config.h and produced a > > libintl.a without references to pthreads. > > Good finding! > > Commenting out HAVE_PTHREAD_API from config.h is also what makes the > option --enable-threads=isoc work as expected on AIX 7.3. > I reconfigured and built gettext with --enable-threads=isoc . libintl.a still contains references to pthread_mutex and friends: $ nm -BCpg libintl.a | grep pthread - U __n_pthreads - U .pthread_mutex_lock - U .pthread_mutex_unlock - U .pthread_mutex_lock - U .pthread_mutex_unlock - U __n_pthreads from files mbrtowc, setlocale_null, and vasnwprintf. I tested on an AIX 7.2.5 system and confirmed that libc does provide the mtx_ symbols: $ nm -BCpg libc.a | grep mtx_ 0 T .mtx_timedlock 160 T .mtx_unlock 256 T .mtx_trylock 416 T .mtx_lock 512 T .mtx_init 736 T .mtx_destroy 80 D mtx_timedlock 92 D mtx_unlock 104 D mtx_trylock 116 D mtx_lock 128 D mtx_init 140 D mtx_destroy Were you suggesting that --enable-threads=isoc would work now or that it would require further changes for a future release? At the moment, configuring gettext with --disable-threads and manually modifying config.h is the only method that produces libintl.a without references to pthreads allowing GCC to build on AIX with NLS enabled. Thanks, David
Re: [PATCH v3 0/2] Replace intl/ with out-of-tree GNU gettext
Don't build with the dependent libraries in tree. Don't build the dependent libraries as shared libraries. The libraries are already built and in /opt/cfarm, as mentioned in the Compile Farm wiki. AIX is not Solaris and not Linux. It doesn't use ELF. AIX shared libraries *ARE* shared object files in archives. Shared object versioning is handled by multiple objects in the same archive. Thanks, David On Thu, Nov 16, 2023 at 4:15 PM Arsen Arsenović wrote: > > Arsen Arsenović writes: > > > [[PGP Signed Part:Good signature from 52C294301EA2C493 Arsen Arsenović > (Gentoo Developer UID) (trust ultimate) created at > 2023-11-16T19:47:16+0100 using EDDSA]] > > > > David Edelsohn writes: > > > >> On Wed, Nov 15, 2023 at 9:22 AM Arsen Arsenović > wrote: > >> > >>> > >>> David Edelsohn writes: > >>> > >>> > GCC had been working on AIX with NLS, using > "--with-included-gettext". > >>> > --disable-nls gets past the breakage, but GCC does not build for me > on > >>> AIX > >>> > with NLS enabled. > >>> > >>> That should still work with gettext 0.22+ extracted in-tree (it should > >>> be fetched by download_prerequisites). > >>> > >>> > A change in dependencies for GCC should have been announced and more > >>> widely > >>> > socialized in the GCC development mailing list, not just GCC patches > >>> > mailing list. > >>> > > >>> > I have tried both the AIX Open Source libiconv and libgettext > package, > >>> and > >>> > the ones that I previously built. Both fail because GCC configure > >>> decides > >>> > to disable NLS, despite being requested, while libcpp is satisfied, > so > >>> > tools in the gcc subdirectory don't link against libiconv and the > build > >>> > fails. With the included gettext, I was able to rely on a > >>> self-consistent > >>> > solution. > >>> > >>> That is interesting. They should be using the same checks. I've > >>> checked trunk and regenerated files on it, and saw no significant diff > >>> (some whitespace changes only). Could you post the config.log of both? > >>> > >>> I've never used AIX. Can I reproduce this on one of the cfarm machines > >>> to poke around? I've tried cfarm119, but that one lacked git, and I > >>> haven't poked around much further due to time constraints. > >>> > >> > >> The AIX system in the Compile Farm has a complete complement of Open > Source > >> software installed. > >> > >> Please ensure that /opt/freeware/bin is in your path. Also, the GCC > Wiki > >> Compile Farm page has build tips that include AIX > >> > >> > https://gcc.gnu.org/wiki/CompileFarm#Services_and_software_installed_on_farm_machines > > > > Thanks, that got me further. > > > >> that recommended --with-included-gettext configuration option. > > > > This flag should still exist and operate the same if gettext is present > > in tree. I've cloned gcc and downloaded prerequisites (via > > contrib/download_prerequisites) and I am trying to configure it now. > > The build failed. After gettext/gmp/... (in-tree hostlibs) get built > and added to library paths, further GCC processes fail to run: > > configure:3305: gcc -g -static-libstdc++ -static-libgcc -Wl,-bbigtoc > conftest.c >&5 > Could not load program > /opt/freeware/libexec/gcc/powerpc-ibm-aix7.3.0.0/10/cc1: > Dependent module > /home/arsen/build/./gmp/.libs/libgmp.a(libgmp.so.10) could not be loaded. > Member libgmp.so.10 is not found in archive > > This seems odd. I am not sure what compels the RTDL (?) to look up .sos > in archives, or how it knows about these archives.. I suspect it's > getting tripped by something in HOST_EXPORTS. > > >> Thanks, David > >> > >> > >>> > >>> TIA, sorry about the inconvenience. Have a lovely day. > >>> > >>> > The current gettext-0.22.3 fails to build for me on AIX. > >>> > > >>> > libcpp configure believes that NLS functions on AIX, but gcc > configure > >>> > fails in its tests of gettext functionality, which leads to an > >>> inconsistent > >>> > configuration and build breakage. > >>> > > >>> > Thanks, David > >>> > >>> > >>> -- > >>> Arsen Arsenović > >>> > > > -- > Arsen Arsenović >
Re: building GNU gettext on AIX
I manually commented out HAVE_PTHREAD_API from config.h and produced a
libintl.a without references to pthreads. Configuring GCC with that custom
libintl.a enables NLS. I now am building GCC with NLS and we will see how
well it functions.
gettext depends on pthreads by default and the versions distributed.
Thanks, David
On Thu, Nov 16, 2023 at 1:01 PM David Edelsohn wrote:
> Bruno,
>
> The issue appears to be that intl/gnulib-lib/{mbrtowc.c,setlocale_null.c}
> include pthread.h based on HAVE_PTHREAD_API, which is defined as 1 in
> intl/config.h build directory despite requesting --disable-pthreads.
>
> Thanks, David
>
> On Thu, Nov 16, 2023 at 11:35 AM David Edelsohn wrote:
>
>> I configured gettext with --disable-pthreads and libintl.a still contains
>> references to pthread_mutex_lock and pthread_mutex_unlock, which causes NLS
>> configure to fail on AIX.
>>
>> How can this be corrected?
>>
>> Thanks, David
>>
>> libintl.a[libgnu_la-mbrtowc.o]:
>>
>> - U __lc_charmap
>>
>> - U errno
>>
>> - U .locale_encoding_classification
>>
>> - U .gl_get_mbtowc_lock
>>
>> - U .pthread_mutex_lock
>>
>> - U .mbtowc
>>
>> - U .pthread_mutex_unlock
>>
>> - U .abort
>>
>> 0 T ._libintl_mbrtowc
>>
>> 1952 D _libintl_mbrtowc
>>
>> libintl.a[libgnu_la-setlocale_null.o]:
>>
>> - U .gl_get_setlocale_null_lock
>>
>> - U .pthread_mutex_lock
>>
>> - U .setlocale
>>
>> - U .strlen
>>
>> - U .memcpy
>>
>> - U .pthread_mutex_unlock
>>
>> - U .abort
>>
>> - U .strcpy
>>
>>336 T ._libintl_setlocale_null_r
>>
>>400 T ._libintl_setlocale_null
>>
>>812 D _libintl_setlocale_null_r
>>
>>824 D _libintl_setlocale_null
>>
>> On Thu, Nov 16, 2023 at 11:00 AM David Edelsohn
>> wrote:
>>
>>> Bruno,
>>>
>>> I have been able to tweak the environment and build gettext and
>>> libintl. With the updated libintl and environment, GCC reliably does not
>>> use NLS.
>>>
>>> The issue is that libintl utilizes pthreads. AIX does not provide no-op
>>> pthread stubs in libc. pthreads is an explicit multilib on AIX.
>>>
>>> It is great that gettext and libintl can be built thread-safe, but GCC
>>> (cc1, gcov, etc.) are not pthreads applications and are not built with
>>> pthreads. Because libintl defaults to pthreads enabled, NLS cannot
>>> function in GCC on AIX by default. The GCC included gettext was built in
>>> the default for GCC libraries, which was not pthreads enabled.
>>>
>>> I can rebuild libintl with --disable-pthreads and I will see if that
>>> works, but the default, distributed libintl library will not allow GCC to
>>> be built with NLS enabled. And, no, GCC on AIX should not be forced to
>>> build with pthreads.
>>>
>>> This is a regression in NLS support in GCC.
>>>
>>> Thanks, David
>>>
>>>
>>> On Wed, Nov 15, 2023 at 5:39 PM Bruno Haible wrote:
>>>
>>>> David Edelsohn wrote:
>>>> > I am using my own install of GCC for a reason.
>>>>
>>>> I have built GNU gettext 0.22.3 in various configurations on the AIX 7.1
>>>> and 7.3 machines in the compilefarm, and haven't encountered issues with
>>>> 'max_align_t' nor with 'getpeername'. So, from my point of view, GNU
>>>> gettext
>>>> works fine on AIX with gcc and xlc (but not ibm-clang, which I haven't
>>>> tested).
>>>>
>>>> You will surely understand that I cannot test a release against a
>>>> compiler
>>>> that exists only on your hard disk.
>>>>
>>>> The hint I gave you, based on the partial logs that you provided, is to
>>>> look at the configure test for intmax_t first.
>>>>
>>>> Bruno
>>>>
>>>>
>>>>
>>>>
Re: building GNU gettext on AIX
Bruno,
The issue appears to be that intl/gnulib-lib/{mbrtowc.c,setlocale_null.c}
include pthread.h based on HAVE_PTHREAD_API, which is defined as 1 in
intl/config.h build directory despite requesting --disable-pthreads.
Thanks, David
On Thu, Nov 16, 2023 at 11:35 AM David Edelsohn wrote:
> I configured gettext with --disable-pthreads and libintl.a still contains
> references to pthread_mutex_lock and pthread_mutex_unlock, which causes NLS
> configure to fail on AIX.
>
> How can this be corrected?
>
> Thanks, David
>
> libintl.a[libgnu_la-mbrtowc.o]:
>
> - U __lc_charmap
>
> - U errno
>
> - U .locale_encoding_classification
>
> - U .gl_get_mbtowc_lock
>
> - U .pthread_mutex_lock
>
> - U .mbtowc
>
> - U .pthread_mutex_unlock
>
> - U .abort
>
> 0 T ._libintl_mbrtowc
>
> 1952 D _libintl_mbrtowc
>
> libintl.a[libgnu_la-setlocale_null.o]:
>
> - U .gl_get_setlocale_null_lock
>
> - U .pthread_mutex_lock
>
> - U .setlocale
>
> - U .strlen
>
> - U .memcpy
>
> - U .pthread_mutex_unlock
>
> - U .abort
>
> - U .strcpy
>
>336 T ._libintl_setlocale_null_r
>
> 400 T ._libintl_setlocale_null
>
>812 D _libintl_setlocale_null_r
>
>824 D _libintl_setlocale_null
>
> On Thu, Nov 16, 2023 at 11:00 AM David Edelsohn wrote:
>
>> Bruno,
>>
>> I have been able to tweak the environment and build gettext and libintl.
>> With the updated libintl and environment, GCC reliably does not use NLS.
>>
>> The issue is that libintl utilizes pthreads. AIX does not provide no-op
>> pthread stubs in libc. pthreads is an explicit multilib on AIX.
>>
>> It is great that gettext and libintl can be built thread-safe, but GCC
>> (cc1, gcov, etc.) are not pthreads applications and are not built with
>> pthreads. Because libintl defaults to pthreads enabled, NLS cannot
>> function in GCC on AIX by default. The GCC included gettext was built in
>> the default for GCC libraries, which was not pthreads enabled.
>>
>> I can rebuild libintl with --disable-pthreads and I will see if that
>> works, but the default, distributed libintl library will not allow GCC to
>> be built with NLS enabled. And, no, GCC on AIX should not be forced to
>> build with pthreads.
>>
>> This is a regression in NLS support in GCC.
>>
>> Thanks, David
>>
>>
>> On Wed, Nov 15, 2023 at 5:39 PM Bruno Haible wrote:
>>
>>> David Edelsohn wrote:
>>> > I am using my own install of GCC for a reason.
>>>
>>> I have built GNU gettext 0.22.3 in various configurations on the AIX 7.1
>>> and 7.3 machines in the compilefarm, and haven't encountered issues with
>>> 'max_align_t' nor with 'getpeername'. So, from my point of view, GNU
>>> gettext
>>> works fine on AIX with gcc and xlc (but not ibm-clang, which I haven't
>>> tested).
>>>
>>> You will surely understand that I cannot test a release against a
>>> compiler
>>> that exists only on your hard disk.
>>>
>>> The hint I gave you, based on the partial logs that you provided, is to
>>> look at the configure test for intmax_t first.
>>>
>>> Bruno
>>>
>>>
>>>
>>>
Re: building GNU gettext on AIX
On Thu, Nov 16, 2023 at 11:58 AM Richard Biener wrote: > > > Am 16.11.2023 um 17:00 schrieb David Edelsohn : > > > Bruno, > > I have been able to tweak the environment and build gettext and libintl. > With the updated libintl and environment, GCC reliably does not use NLS. > > The issue is that libintl utilizes pthreads. AIX does not provide no-op > pthread stubs in libc. pthreads is an explicit multilib on AIX. > > It is great that gettext and libintl can be built thread-safe, but GCC > (cc1, gcov, etc.) are not pthreads applications and are not built with > pthreads. Because libintl defaults to pthreads enabled, NLS cannot > function in GCC on AIX by default. The GCC included gettext was built in > the default for GCC libraries, which was not pthreads enabled. > > I can rebuild libintl with --disable-pthreads and I will see if that > works, but the default, distributed libintl library will not allow GCC to > be built with NLS enabled. And, no, GCC on AIX should not be forced to > build with pthreads. > > This is a regression in NLS support in GCC. > > > If that’s for the in-tree libintl we can arrange configure to pass down > —disable-pthreads like we adjust configure args for gmp and friends as well. > The latest issue is that a few files in gettext ignore --disable-pthreads and creates a dependency on pthread_mutex. David > > Richard > > Thanks, David > > > On Wed, Nov 15, 2023 at 5:39 PM Bruno Haible wrote: > >> David Edelsohn wrote: >> > I am using my own install of GCC for a reason. >> >> I have built GNU gettext 0.22.3 in various configurations on the AIX 7.1 >> and 7.3 machines in the compilefarm, and haven't encountered issues with >> 'max_align_t' nor with 'getpeername'. So, from my point of view, GNU >> gettext >> works fine on AIX with gcc and xlc (but not ibm-clang, which I haven't >> tested). >> >> You will surely understand that I cannot test a release against a compiler >> that exists only on your hard disk. >> >> The hint I gave you, based on the partial logs that you provided, is to >> look at the configure test for intmax_t first. >> >> Bruno >> >> >> >>
Re: building GNU gettext on AIX
I configured gettext with --disable-pthreads and libintl.a still contains references to pthread_mutex_lock and pthread_mutex_unlock, which causes NLS configure to fail on AIX. How can this be corrected? Thanks, David libintl.a[libgnu_la-mbrtowc.o]: - U __lc_charmap - U errno - U .locale_encoding_classification - U .gl_get_mbtowc_lock - U .pthread_mutex_lock - U .mbtowc - U .pthread_mutex_unlock - U .abort 0 T ._libintl_mbrtowc 1952 D _libintl_mbrtowc libintl.a[libgnu_la-setlocale_null.o]: - U .gl_get_setlocale_null_lock - U .pthread_mutex_lock - U .setlocale - U .strlen - U .memcpy - U .pthread_mutex_unlock - U .abort - U .strcpy 336 T ._libintl_setlocale_null_r 400 T ._libintl_setlocale_null 812 D _libintl_setlocale_null_r 824 D _libintl_setlocale_null On Thu, Nov 16, 2023 at 11:00 AM David Edelsohn wrote: > Bruno, > > I have been able to tweak the environment and build gettext and libintl. > With the updated libintl and environment, GCC reliably does not use NLS. > > The issue is that libintl utilizes pthreads. AIX does not provide no-op > pthread stubs in libc. pthreads is an explicit multilib on AIX. > > It is great that gettext and libintl can be built thread-safe, but GCC > (cc1, gcov, etc.) are not pthreads applications and are not built with > pthreads. Because libintl defaults to pthreads enabled, NLS cannot > function in GCC on AIX by default. The GCC included gettext was built in > the default for GCC libraries, which was not pthreads enabled. > > I can rebuild libintl with --disable-pthreads and I will see if that > works, but the default, distributed libintl library will not allow GCC to > be built with NLS enabled. And, no, GCC on AIX should not be forced to > build with pthreads. > > This is a regression in NLS support in GCC. > > Thanks, David > > > On Wed, Nov 15, 2023 at 5:39 PM Bruno Haible wrote: > >> David Edelsohn wrote: >> > I am using my own install of GCC for a reason. >> >> I have built GNU gettext 0.22.3 in various configurations on the AIX 7.1 >> and 7.3 machines in the compilefarm, and haven't encountered issues with >> 'max_align_t' nor with 'getpeername'. So, from my point of view, GNU >> gettext >> works fine on AIX with gcc and xlc (but not ibm-clang, which I haven't >> tested). >> >> You will surely understand that I cannot test a release against a compiler >> that exists only on your hard disk. >> >> The hint I gave you, based on the partial logs that you provided, is to >> look at the configure test for intmax_t first. >> >> Bruno >> >> >> >>
Re: building GNU gettext on AIX
Bruno, I have been able to tweak the environment and build gettext and libintl. With the updated libintl and environment, GCC reliably does not use NLS. The issue is that libintl utilizes pthreads. AIX does not provide no-op pthread stubs in libc. pthreads is an explicit multilib on AIX. It is great that gettext and libintl can be built thread-safe, but GCC (cc1, gcov, etc.) are not pthreads applications and are not built with pthreads. Because libintl defaults to pthreads enabled, NLS cannot function in GCC on AIX by default. The GCC included gettext was built in the default for GCC libraries, which was not pthreads enabled. I can rebuild libintl with --disable-pthreads and I will see if that works, but the default, distributed libintl library will not allow GCC to be built with NLS enabled. And, no, GCC on AIX should not be forced to build with pthreads. This is a regression in NLS support in GCC. Thanks, David On Wed, Nov 15, 2023 at 5:39 PM Bruno Haible wrote: > David Edelsohn wrote: > > I am using my own install of GCC for a reason. > > I have built GNU gettext 0.22.3 in various configurations on the AIX 7.1 > and 7.3 machines in the compilefarm, and haven't encountered issues with > 'max_align_t' nor with 'getpeername'. So, from my point of view, GNU > gettext > works fine on AIX with gcc and xlc (but not ibm-clang, which I haven't > tested). > > You will surely understand that I cannot test a release against a compiler > that exists only on your hard disk. > > The hint I gave you, based on the partial logs that you provided, is to > look at the configure test for intmax_t first. > > Bruno > > > >
Re: building GNU gettext on AIX
On Wed, Nov 15, 2023 at 4:22 PM Bruno Haible wrote:
> David Edelsohn wrote:
> > When I try to configure gettext-0.22.3, I receive the following error:
> >
> > checking for socklen_t equivalent... configure: error: Cannot find a type
> > to use in place of socklen_t
> >
> > configure: error:
> > /nasfarm/edelsohn/src/gettext-0.22.3/libtextstyle/configure failed for
> > libtextstyle
> >
> >
> > configure:43943: /nasfarm/edelsohn/install/GCC12/bin/gcc -c -g -O2
> > -D_THREAD_SAFE
> > conftest.c >&5
> >
> > conftest.c:112:18: error: two or more data types in declaration
> specifiers
> >
> > 112 | #define intmax_t long long
> >
> > | ^~~~
> >
> > conftest.c:112:23: error: two or more data types in declaration
> specifiers
> >
> > 112 | #define intmax_t long long
> >
> > | ^~~~
> >
> > In file included from conftest.c:212:
> >
> > conftest.c:214:24: error: conflicting types for 'ngetpeername'; have
> > 'int(int, void *, long unsigned int *)'
> >
> > 214 |int getpeername (int, void *, unsigned long
> int
> > *);
> >
> > |^~~
> >
> >
> /nasfarm/edelsohn/install/GCC12/lib/gcc/powerpc-ibm-aix7.2.5.0/12.1.1/include-fixed/sys/socket.h:647:9:
> > note: previous declaration of 'ngetpeername' with type 'int(int, struct
> > sockaddr * restrict, socklen_t * restrict)' {aka 'int(int, struct
> > sockaddr * restrict, long unsigned int * restrict)'}
> >
> > 647 | int getpeername(int, struct sockaddr *__restrict__, socklen_t
> > *__restrict__);
> >
> > | ^~~
> >
> >
> > configure and config.h seems to get itself confused about types.
>
> There seem to be two problems, both related to the include files of
> your compiler:
>
> - The configure test "checking for intmax_t..." must have found the
> answer "no". But on a modern system, should be defining
> intmax_t already.
>
> - This configure test that tries to find the getpeername declaration,
> but cannot find it (maybe because of the first problem?):
>
>
>
> for arg2 in "struct sockaddr" void; do
>for t in int size_t "unsigned int" "long int" "unsigned long
> int"; do
> cat confdefs.h - <<_ACEOF >conftest.$ac_ext
> /* end confdefs.h. */
> #include
>#include
>
>int getpeername (int, $arg2 *, $t *);
> int
> main (void)
> {
> $t len;
> getpeername (0, 0, );
> ;
> return 0;
> }
> _ACEOF
> if ac_fn_c_try_compile "$LINENO"
> then :
> gl_cv_socklen_t_equiv="$t"
> fi
>
>
>
> I would concentrate on the first problem. If you don't get it fixed, then
> I'd
> suggest to try 'gcc' from the AIX Toolbox [1] or 'xlc' (as an IBM product)
> instead of 'gcc' (that looks like you built it yourself).
>
> Bruno
>
> [1]
> https://public.dhe.ibm.com/aix/freeSoftware/aixtoolbox/SPECS/gcc12-12.3.0-1.spec
Bruno,
I am using my own install of GCC for a reason. The build of GCC works for
everything else, including bootstrap of GCC, GDB, GMP, etc. The only
problem is gettext.
Thanks, David
Re: [PATCH v3 0/2] Replace intl/ with out-of-tree GNU gettext
On Wed, Nov 15, 2023 at 9:22 AM Arsen Arsenović wrote: > > David Edelsohn writes: > > > GCC had been working on AIX with NLS, using "--with-included-gettext". > > --disable-nls gets past the breakage, but GCC does not build for me on > AIX > > with NLS enabled. > > That should still work with gettext 0.22+ extracted in-tree (it should > be fetched by download_prerequisites). > > > A change in dependencies for GCC should have been announced and more > widely > > socialized in the GCC development mailing list, not just GCC patches > > mailing list. > > > > I have tried both the AIX Open Source libiconv and libgettext package, > and > > the ones that I previously built. Both fail because GCC configure > decides > > to disable NLS, despite being requested, while libcpp is satisfied, so > > tools in the gcc subdirectory don't link against libiconv and the build > > fails. With the included gettext, I was able to rely on a > self-consistent > > solution. > > That is interesting. They should be using the same checks. I've > checked trunk and regenerated files on it, and saw no significant diff > (some whitespace changes only). Could you post the config.log of both? > GCC configured with --with-libintl-prefix and --with-libiconv-prefix libcpp/config.log: configure:7610: checking for GNU gettext in libc configure:7639: /nasfarm/edelsohn/install/GCC12/bin/gcc -std=gnu99 -o conftest -g -static-libstdc++ -static-libgcc -Wl,-bbigtoc conftest.c >&5 conftest.c:71:10: fatal error: libintl.h: No such file or directory 71 | #include | ^~~ configure:8318: checking for GNU gettext in libintl configure:8355: /nasfarm/edelsohn/install/GCC12/bin/gcc -std=gnu99 -o conftest -g -I/nasfarm/edelsohn/install/include -static-libstdc++ -static-libgcc -Wl,-bbigtoc conftest.c /nasfarm/edelsohn/install/lib/ libintl.a >&5 ld: 0711-317 ERROR: Undefined symbol: .libiconv_open ld: 0711-317 ERROR: Undefined symbol: .libiconv_set_relocation_prefix ld: 0711-317 ERROR: Undefined symbol: .libiconv_close ld: 0711-317 ERROR: Undefined symbol: .libiconv ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information. collect2: error: ld returned 8 exit status configure:8355: $? = 1 configure:8392: /nasfarm/edelsohn/install/GCC12/bin/gcc -std=gnu99 -o conftest -g -I/nasfarm/edelsohn/install/include -static-libstdc++ -static-libgcc -Wl,-bbigtoc conftest.c /nasfarm/edelsohn/install/lib/ libintl.a /nasfarm/edelsohn/install/lib/libiconv.a >&5 configure:8392: $? = 0 configure:8405: result: yes configure:8440: checking whether to use NLS configure:8442: result: yes configure:8445: checking where the gettext function comes from configure:8456: result: external libintl configure:8464: checking how to link with libintl configure:8466: result: /nasfarm/edelsohn/install/lib/libintl.a /nasfarm/edelsohn/install/lib/libiconv.a configure:8525: checking whether NLS is requested configure:8531: result: yes gcc/config.log: configure:14002: checking for GNU gettext in libc configure:14031: /nasfarm/edelsohn/install/GCC12/bin/g++ -std=c++11 -o conftest -g-static-libstdc++ -static-libgcc -Wl,-bbigtoc conftest.cpp >&5 conftest.cpp:196:10: fatal error: libintl.h: No such file or directory 196 | #include | ^~~ configure:14710: checking for GNU gettext in libintl configure:14747: /nasfarm/edelsohn/install/GCC12/bin/g++ -std=c++11 -o conftest -g-I/nasfarm/edelsohn/install/include -static-libstdc++ -static-libgcc -Wl,-bbigtoc conftest.cpp /nasfarm/edelsohn/install/lib/ libintl.a >&5 ld: 0711-317 ERROR: Undefined symbol: .libiconv_open ld: 0711-317 ERROR: Undefined symbol: .libiconv_set_relocation_prefix ld: 0711-317 ERROR: Undefined symbol: .libiconv_close ld: 0711-317 ERROR: Undefined symbol: .libiconv ld: 0711-345 Use the -bloadmap or -bnoquiet option to obtain more information. collect2: error: ld returned 8 exit status configure:14747: $? = 1 configure:14797: result: no configure:14832: checking whether to use NLS configure:14834: result: no configure:14917: checking whether NLS is requested configure:14920: result: no > I've never used AIX. Can I reproduce this on one of the cfarm machines > to poke around? I've tried cfarm119, but that one lacked git, and I > haven't poked around much further due to time constraints. > > TIA, sorry about the inconvenience. Have a lovely day. > > > The current gettext-0.22.3 fails to build for me on AIX. > > > > libcpp configure believes that NLS functions on AIX, but gcc configure > > fails in its tests of gettext functionality, which leads to an > inconsistent > > configuration and build breakage. > > > > Thanks, David > > > -- > Arsen Arsenović >
Re: building GNU gettext on AIX
When I try to configure gettext-0.22.3, I receive the following error:
checking for socklen_t equivalent... configure: error: Cannot find a type
to use in place of socklen_t
configure: error:
/nasfarm/edelsohn/src/gettext-0.22.3/libtextstyle/configure failed for
libtextstyle
configure:43943: /nasfarm/edelsohn/install/GCC12/bin/gcc -c -g -O2
-D_THREAD_SAFE
conftest.c >&5
conftest.c:112:18: error: two or more data types in declaration specifiers
112 | #define intmax_t long long
| ^~~~
conftest.c:112:23: error: two or more data types in declaration specifiers
112 | #define intmax_t long long
| ^~~~
In file included from conftest.c:212:
conftest.c:214:24: error: conflicting types for 'ngetpeername'; have
'int(int, void *, long unsigned int *)'
214 |int getpeername (int, void *, unsigned long int
*);
|^~~
/nasfarm/edelsohn/install/GCC12/lib/gcc/powerpc-ibm-aix7.2.5.0/12.1.1/include-fixed/sys/socket.h:647:9:
note: previous declaration of 'ngetpeername' with type 'int(int, struct
sockaddr * restrict, socklen_t * restrict)' {aka 'int(int, struct
sockaddr * restrict, long unsigned int * restrict)'}
647 | int getpeername(int, struct sockaddr *__restrict__, socklen_t
*__restrict__);
| ^~~
configure and config.h seems to get itself confused about types.
David
On Wed, Nov 15, 2023 at 7:29 AM Bruno Haible wrote:
> [CCing bug-gettext]
>
> David Edelsohn wrote in
> <https://gcc.gnu.org/pipermail/gcc-patches/2023-November/636558.html>:
> > The current gettext-0.22.3 fails to build for me on AIX.
>
> Here are some hints to get a successful build of GNU gettext on AIX:
>
> 1. Set the recommended environment variables before running configure:
>https://gitlab.com/ghwiki/gnow-how/-/wikis/Platforms/Configuration
>
>Namely:
>* for a 32-bit build with gcc:
> CC=gcc
> CXX=g++
> CPPFLAGS="-I$PREFIX/include"
> LDFLAGS="-L$PREFIX/lib"
> unset AR NM
>* for a 32-bit build with xlc:
> CC="xlc -qthreaded -qtls"
> CXX="xlC -qthreaded -qtls"
> CPPFLAGS="-I$PREFIX/include"
> LDFLAGS="-L$PREFIX/lib"
> unset AR NM
>* for a 64-bit build with gcc:
> CC="gcc -maix64"
> CXX="g++ -maix64"
> CPPFLAGS="-I$PREFIX/include"
> LDFLAGS="-L$PREFIX/lib"
> AR="ar -X 64"; NM="nm -X 64 -B"
>* for a 64-bit build with xlc:
> CC="xlc -q64 -qthreaded -qtls"
> CXX="xlC -q64 -qthreaded -qtls"
> CPPFLAGS="-I$PREFIX/include"
> LDFLAGS="-L$PREFIX/lib"
> AR="ar -X 64"; NM="nm -X 64 -B"
>
>where $PREFIX is the value that you pass to the --prefix configure
> option.
>
>Rationale: you can run into all sorts of problems if you choose compiler
>options at random and haven't experience with compiler options on that
>platform.
>
> 2. Don't use ibm-clang.
>
>Rationale: It's broken.
>
> 3. Don't use -Wall with gcc 10.3.
>
>Rationale: If you specify -Wall, gettext's configure adds -fanalyzer,
> which
>has excessive memory requirements in gcc 10.x. In particular, on AIX, it
>makes cc1 crash while compiling regex.c after it has consumed 1 GiB of
> RAM.
>
> 4. Avoid using a --prefix that contains earlier installations of the same
>package.
>
>Rationale: Because the AIX linker hardcodes directory names in shared
>libraries, GNU libtool has a peculiar configuration on AIX. It ends up
>mixing the in-build-tree libraries with the libraries in the install
>locations, leading to all sorts of errors.
>
>If you really need to use a --prefix that contains an earlier
>installation of the same package:
> - Either use --disable-shared and remove libgettextlib.a and
>libgettextsrc.a from $PREFIX/lib before starting the build.
> - Or use a mix of "make -k", "make -k install" and ad-hoc workarounds
>that cannot be described in a general way.
>
> Bruno
>
>
>
>
Re: [PATCH v3 0/2] Replace intl/ with out-of-tree GNU gettext
On Wed, Nov 15, 2023 at 9:22 AM Arsen Arsenović wrote: > > David Edelsohn writes: > > > GCC had been working on AIX with NLS, using "--with-included-gettext". > > --disable-nls gets past the breakage, but GCC does not build for me on > AIX > > with NLS enabled. > > That should still work with gettext 0.22+ extracted in-tree (it should > be fetched by download_prerequisites). > > > A change in dependencies for GCC should have been announced and more > widely > > socialized in the GCC development mailing list, not just GCC patches > > mailing list. > > > > I have tried both the AIX Open Source libiconv and libgettext package, > and > > the ones that I previously built. Both fail because GCC configure > decides > > to disable NLS, despite being requested, while libcpp is satisfied, so > > tools in the gcc subdirectory don't link against libiconv and the build > > fails. With the included gettext, I was able to rely on a > self-consistent > > solution. > > That is interesting. They should be using the same checks. I've > checked trunk and regenerated files on it, and saw no significant diff > (some whitespace changes only). Could you post the config.log of both? > > I've never used AIX. Can I reproduce this on one of the cfarm machines > to poke around? I've tried cfarm119, but that one lacked git, and I > haven't poked around much further due to time constraints. > The AIX system in the Compile Farm has a complete complement of Open Source software installed. Please ensure that /opt/freeware/bin is in your path. Also, the GCC Wiki Compile Farm page has build tips that include AIX https://gcc.gnu.org/wiki/CompileFarm#Services_and_software_installed_on_farm_machines that recommended --with-included-gettext configuration option. Thanks, David > > TIA, sorry about the inconvenience. Have a lovely day. > > > The current gettext-0.22.3 fails to build for me on AIX. > > > > libcpp configure believes that NLS functions on AIX, but gcc configure > > fails in its tests of gettext functionality, which leads to an > inconsistent > > configuration and build breakage. > > > > Thanks, David > > > -- > Arsen Arsenović >
Re: [PATCH v3 0/2] Replace intl/ with out-of-tree GNU gettext
On Tue, Nov 14, 2023 at 6:09 PM Arsen Arsenović wrote: > Hi David, > > David Edelsohn writes: > > > Arsen, > > > > Unfortunately this broke bootstrap on AIX. > > > > I had not seen this series of patches. > > I've added Bruno to CC as the libintl maintainer, to keep him in the > loop. Could you provide some extra information w.r.t. the failure mode? > > I'll try to investigate ASAP. > > In the meanwhile, does disabling NLS etc work? > > Thanks in advance, have a lovely night. > > GCC had been working on AIX with NLS, using "--with-included-gettext". --disable-nls gets past the breakage, but GCC does not build for me on AIX with NLS enabled. A change in dependencies for GCC should have been announced and more widely socialized in the GCC development mailing list, not just GCC patches mailing list. I have tried both the AIX Open Source libiconv and libgettext package, and the ones that I previously built. Both fail because GCC configure decides to disable NLS, despite being requested, while libcpp is satisfied, so tools in the gcc subdirectory don't link against libiconv and the build fails. With the included gettext, I was able to rely on a self-consistent solution. The current gettext-0.22.3 fails to build for me on AIX. libcpp configure believes that NLS functions on AIX, but gcc configure fails in its tests of gettext functionality, which leads to an inconsistent configuration and build breakage. Thanks, David
Re: [PATCH v3 0/2] Replace intl/ with out-of-tree GNU gettext
Arsen, Unfortunately this broke bootstrap on AIX. I had not seen this series of patches. David
Re: [PATCH v3] Control flow redundancy hardening
SPARC target header requires memmodel.h. David * gimple-harden-control-flow.cc: Include memmodel.h. *diff --git a/gcc/gimple-harden-control-flow.cc b/gcc/gimple-harden-control-flow.cc* *index 1b345dab766..441df5ac21c 100644* *--- a/gcc/gimple-harden-control-flow.cc* *+++ b/gcc/gimple-harden-control-flow.cc* @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see #include "system.h" #include "coretypes.h" #include "backend.h" +#include "memmodel.h" #include "tm_p.h" #include "tree.h" #include "fold-const.h" On Fri, Oct 20, 2023 at 2:18 PM David Edelsohn wrote: > This patch broke bootstrap on AIX because ASM_GENERATE_INTERNAL_LABEL can > embed target-specific functions. Fixed by including tm_p.h. > > Thanks, David > > * gimple-harden-control-flow.cc: Include tm_p.h. > > > *diff --git a/gcc/gimple-harden-control-flow.cc > b/gcc/gimple-harden-control-flow.cc* > > *index 5c28fd07f33..1b345dab766 100644* > > *--- a/gcc/gimple-harden-control-flow.cc* > > *+++ b/gcc/gimple-harden-control-flow.cc* > > @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see > > #include "system.h" > > #include "coretypes.h" > > #include "backend.h" > > +#include "tm_p.h" > > #include "tree.h" > > #include "fold-const.h" > > #include "gimple.h" >
Re: [PATCH v3] Control flow redundancy hardening
This patch broke bootstrap on AIX because ASM_GENERATE_INTERNAL_LABEL can embed target-specific functions. Fixed by including tm_p.h. Thanks, David * gimple-harden-control-flow.cc: Include tm_p.h. *diff --git a/gcc/gimple-harden-control-flow.cc b/gcc/gimple-harden-control-flow.cc* *index 5c28fd07f33..1b345dab766 100644* *--- a/gcc/gimple-harden-control-flow.cc* *+++ b/gcc/gimple-harden-control-flow.cc* @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see #include "system.h" #include "coretypes.h" #include "backend.h" +#include "tm_p.h" #include "tree.h" #include "fold-const.h" #include "gimple.h"
Re: [PATCH v2] swap: Fix incorrect lane extraction by vec_extract() [PR106770]
[Resending from correct email.]
Hi, Surya
Thanks for working on this issue and creating a patch.
It helps if you explicitly send patches to Segher and me, and copy
gcc-patches.
+/* Return true if insn is a non-permuting load/store. */
+static bool
+non_permuting_mem_insn (swap_web_entry *insn_entry, unsigned int i)
+{
+ return (insn_entry[i].special_handling == SH_NOSWAP_LD ||
+ insn_entry[i].special_handling == SH_NOSWAP_ST);
+}
The logical operator || should be at the beginning of the line, not
the end. That also ensures correct formatting and indentation. The
|| should be under the "i" of insn on the line above.
It also generally simplifies review to have the ChangeLog in the same
order as changes in the file. The order of the files relative to the
patch may not be the same, but the ChangeLog entries should be in
sequential order relative to the file.
This patch is okay with the logical operator formatting change.
Thanks, David
Re: [PATCH v2] swap: Fix incorrect lane extraction by vec_extract() [PR106770]
Hi, Surya
Thanks for working on this issue and creating a patch.
It helps if you explicitly send patches to Segher and me, and copy gcc-patches.
+/* Return true if insn is a non-permuting load/store. */
+static bool
+non_permuting_mem_insn (swap_web_entry *insn_entry, unsigned int i)
+{
+ return (insn_entry[i].special_handling == SH_NOSWAP_LD ||
+ insn_entry[i].special_handling == SH_NOSWAP_ST);
+}
The logical operator || should be at the beginning of the line, not
the end. That also ensures correct formatting and indentation. The
|| should be under the "i" of insn on the line above.
It also generally simplifies review to have the ChangeLog in the same
order as changes in the file. The order of the files relative to the
patch may not be the same, but the ChangeLog entries should be in
sequential order relative to the file.
This patch is okay with the logical operator formatting change.
Thanks, David
Re: [PATCH] PR target/111778 - Fix undefined shifts in PowerPC compiler
On Thu, Oct 12, 2023 at 4:24 AM Michael Meissner
wrote:
> I was building a cross compiler to PowerPC on my x86_86 workstation with
> the
> latest version of GCC on October 11th. I could not build the compiler on
> the
> x86_64 system as it died in building libgcc. I looked into it, and I
> discovered the compiler was recursing until it ran out of stack space. If
> I
> build a native compiler with the same sources on a PowerPC system, it
> builds
> fine.
>
> I traced this down to a change made around October 10th:
>
> | commit 8f1a70a4fbcc6441c70da60d4ef6db1e5635e18a (HEAD)
> | Author: Jiufu Guo
> | Date: Tue Jan 10 20:52:33 2023 +0800
> |
> | rs6000: build constant via li/lis;rldicl/rldicr
> |
> | If a constant is possible left/right cleaned on a rotated value from
> | a negative value of "li/lis". Then, using "li/lis ; rldicl/rldicr"
> | to build the constant.
>
> The code was doing a -1 << 64 which is undefined behavior because different
> machines produce different results. On the x86_64 system, (-1 << 64)
> produces
> -1 while on a PowerPC 64-bit system, (-1 << 64) produces 0. The x86_64
> then
> recurses until the stack runs out of space.
>
> If I apply this patch, the compiler builds fine on both x86_64 as a PowerPC
> crosss compiler and on a native PowerPC system.
>
> Can I check this into the master branch to fix the problem?
>
Thanks for finding and debugging this problem. This is okay.
Thanks, David
>
> 2023-10-12 Michael Meissner
>
> gcc/
>
> PR target/111778
> * config/rs6000/rs6000.cc (can_be_built_by_li_lis_and_rldicl):
> Protect
> code from shifts that are undefined.
> (can_be_built_by_li_lis_and_rldicr): Likewise.
> (can_be_built_by_li_and_rldic): Protect code from shifts that
> undefined. Also replace uses of 1ULL with HOST_WIDE_INT_1U.
>
> ---
> gcc/config/rs6000/rs6000.cc | 29 ++---
> 1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 2828f01413c..cc24dd5301e 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10370,6 +10370,11 @@ can_be_built_by_li_lis_and_rldicl (HOST_WIDE_INT
> c, int *shift,
>/* Leading zeros may be cleaned by rldicl with a mask. Change leading
> zeros
> to ones and then recheck it. */
>int lz = clz_hwi (c);
> +
> + /* If lz == 0, the left shift is undefined. */
> + if (!lz)
> +return false;
> +
>HOST_WIDE_INT unmask_c
> = c | (HOST_WIDE_INT_M1U << (HOST_BITS_PER_WIDE_INT - lz));
>int n;
> @@ -10398,6 +10403,11 @@ can_be_built_by_li_lis_and_rldicr (HOST_WIDE_INT
> c, int *shift,
>/* Tailing zeros may be cleaned by rldicr with a mask. Change tailing
> zeros
> to ones and then recheck it. */
>int tz = ctz_hwi (c);
> +
> + /* If tz == HOST_BITS_PER_WIDE_INT, the left shift is undefined. */
> + if (tz >= HOST_BITS_PER_WIDE_INT)
> +return false;
> +
>HOST_WIDE_INT unmask_c = c | ((HOST_WIDE_INT_1U << tz) - 1);
>int n;
>if (can_be_rotated_to_lowbits (~unmask_c, 15, )
> @@ -10428,8 +10438,15 @@ can_be_built_by_li_and_rldic (HOST_WIDE_INT c,
> int *shift, HOST_WIDE_INT *mask)
> right bits are shifted as 0's, and left 1's(and x's) are cleaned. */
>int tz = ctz_hwi (c);
>int lz = clz_hwi (c);
> +
> + /* If lz == HOST_BITS_PER_WIDE_INT, the left shift is undefined. */
> + if (lz >= HOST_BITS_PER_WIDE_INT)
> +return false;
> +
>int middle_ones = clz_hwi (~(c << lz));
> - if (tz + lz + middle_ones >= ones)
> + if (tz + lz + middle_ones >= ones
> + && (tz - lz) < HOST_BITS_PER_WIDE_INT
> + && tz < HOST_BITS_PER_WIDE_INT)
> {
>*mask = ((1LL << (HOST_BITS_PER_WIDE_INT - tz - lz)) - 1LL) << tz;
>*shift = tz;
> @@ -10440,7 +10457,8 @@ can_be_built_by_li_and_rldic (HOST_WIDE_INT c, int
> *shift, HOST_WIDE_INT *mask)
>int leading_ones = clz_hwi (~c);
>int tailing_ones = ctz_hwi (~c);
>int middle_zeros = ctz_hwi (c >> tailing_ones);
> - if (leading_ones + tailing_ones + middle_zeros >= ones)
> + if (leading_ones + tailing_ones + middle_zeros >= ones
> + && middle_zeros < HOST_BITS_PER_WIDE_INT)
> {
>*mask = ~(((1ULL << middle_zeros) - 1ULL) << tailing_ones);
>*shift = tailing_ones + middle_zeros;
> @@ -10450,10 +10468,15 @@ can_be_built_by_li_and_rldic (HOST_WIDE_INT c,
> int *shift, HOST_WIDE_INT *mask)
>/* xx1..1xx: --> xx0..01..1xx: some 1's(following x's) are cleaned. */
>/* Get the position for the first bit of successive 1.
> The 24th bit would be in successive 0 or 1. */
> - HOST_WIDE_INT low_mask = (1LL << 24) - 1LL;
> + HOST_WIDE_INT low_mask = (HOST_WIDE_INT_1U << 24) - HOST_WIDE_INT_1U;
>int pos_first_1 = ((c & (low_mask + 1)) == 0)
> ? clz_hwi (c & low_mask)
> : HOST_BITS_PER_WIDE_INT - ctz_hwi (~(c | low_mask));
> +
> + /* Make
Re: [PATCH] early outs for functions in rs6000.cc
On Tue, Oct 10, 2023 at 9:29 PM Jiufu Guo wrote:
> Hi,
>
> There are some piece of code like below in rs6000.cc:
>
> ...
> if (xx)
>return x;
> else if (yy)
>return y;
> ... //else if chain
> else
>return d;
>
> Using early outs would be more preferable for this kind of code.
>
Why?
This seems like premature optimization.
Thanks, David
>
> The whole function rs6000_emit_set_long_const and num_insns_constant_gpr
> are refined. And similar code are also restuctured in rs6000.cc.
>
> This patch pass bootstrap and regtest on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu Guo)
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.cc (vspltis_shifted): Adopt early outs.
> (rs6000_cost_data::determine_suggested_unroll_factor): Likewise.
> (num_insns_constant_gpr): Likewise.
> (easy_altivec_constant): Likewise.
> (output_vec_const_move): Likewise.
> (rs6000_expand_vector_set): Likewise.
> (rs6000_legitimize_address): Likewise.
> (rs6000_emit_set_long_const): Likewise.
> (rs6000_preferred_reload_class): Likewise.
> (rs6000_can_change_mode_class): Likewise.
> (rs6000_output_move_128bit): Likewise.
> (rs6000_emit_vector_cond_expr): Likewise.
> (adjacent_mem_locations): Likewise.
> (is_fusable_store): Likewise.
> (insn_terminates_group_p): Likewise.
> (rs6000_elf_reloc_rw_mask): Likewise.
> (rs6000_rtx_costs): Likewise.
> (rs6000_scalar_mode_supported_p): Likewise.
> (rs6000_update_ipa_fn_target_info): Likewise.
> (reg_to_non_prefixed): Likewise.
> (rs6000_split_logical_inner): Likewise.
> (rs6000_opaque_type_invalid_use_p): Likewise.
>
> ---
> gcc/config/rs6000/rs6000.cc | 495 ++--
> 1 file changed, 249 insertions(+), 246 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index d10d22a5816..43681d9eb08 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -5522,24 +5522,22 @@
> rs6000_cost_data::determine_suggested_unroll_factor (loop_vec_info
> loop_vinfo)
> to vectorize it with the unrolled VF any more if the actual
> iteration
> count is in between. */
> return 1;
> - else
> -{
> - unsigned int epil_niter_unr = est_niter % unrolled_vf;
> - unsigned int epil_niter = est_niter % vf;
> - /* Even if we have partial vector support, it can be still
> inefficent
> -to calculate the length when the iteration count is unknown, so
> -only expect it's good to unroll when the epilogue iteration count
> -is not bigger than VF (only one time length calculation). */
> - if (LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
> - && epil_niter_unr <= vf)
> - return uf;
> - /* Without partial vector support, conservatively unroll this when
> -the epilogue iteration count is less than the original one
> -(epilogue execution time wouldn't be longer than before). */
> - else if (!LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
> - && epil_niter_unr <= epil_niter)
> - return uf;
> -}
> +
> + unsigned int epil_niter_unr = est_niter % unrolled_vf;
> + unsigned int epil_niter = est_niter % vf;
> + /* Even if we have partial vector support, it can be still inefficent
> + to calculate the length when the iteration count is unknown, so
> + only expect it's good to unroll when the epilogue iteration count
> + is not bigger than VF (only one time length calculation). */
> + if (LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo) && epil_niter_unr
> <= vf)
> +return uf;
> +
> + /* Without partial vector support, conservatively unroll this when
> + the epilogue iteration count is less than the original one
> + (epilogue execution time wouldn't be longer than before). */
> + if (!LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo)
> + && epil_niter_unr <= epil_niter)
> +return uf;
>
>return 1;
> }
> @@ -6044,35 +6042,31 @@ num_insns_constant_gpr (HOST_WIDE_INT value)
> return 1;
>
>/* constant loadable with addis */
> - else if ((value & 0x) == 0
> - && (value >> 31 == -1 || value >> 31 == 0))
> + if ((value & 0x) == 0 && (value >> 31 == -1 || value >> 31 == 0))
> return 1;
>
>/* PADDI can support up to 34 bit signed integers. */
> - else if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (value))
> + if (TARGET_PREFIXED && SIGNED_INTEGER_34BIT_P (value))
> return 1;
>
> - else if (TARGET_POWERPC64)
> -{
> - HOST_WIDE_INT low = sext_hwi (value, 32);
> - HOST_WIDE_INT high = value >> 31;
> + if (!TARGET_POWERPC64)
> +return 2;
>
> - if (high == 0 || high == -1)
> - return 2;
> + /* TARGET_POWERPC64 */
> + HOST_WIDE_INT low = sext_hwi (value, 32);
> + HOST_WIDE_INT high = value >> 31;
> +
> + if
Re: [PATCH-2, rs6000] Enable vector mode for memory equality compare [PR111449]
On Tue, Oct 10, 2023 at 4:23 AM HAO CHEN GUI wrote: > Hi David, > > Thanks for your review comments. > > 在 2023/10/9 23:42, David Edelsohn 写道: > > #define MOVE_MAX (! TARGET_POWERPC64 ? 4 : 8) > > #define MAX_MOVE_MAX 8 > > +#define MOVE_MAX_PIECES (!TARGET_POWERPC64 ? 4 : 16) > > +#define COMPARE_MAX_PIECES (!TARGET_POWERPC64 ? 4 : 16) > > > > > > How are the definitions of MOVE_MAX_PIECES and COMPARE_MAX_PIECES > determined? The email does not provide any explanation for the > implementation. The rest of the patch is related to vector support, but > vector support is not dependent on TARGET_POWERPC64. > > By default, MOVE_MAX_PIECES and COMPARE_MAX_PIECES is set the same value > as MOVE_MAX. The move and compare instructions are required in > compare_by_pieces, those macros are set to 16 byte when supporting > vector mode (V16QImode). The problem is rs6000 hasn't supported TImode > for "-m32". We discussed it in issue 1307. TImode will be used for > move when MOVE_MAX_PIECES is set to 16. But TImode isn't supported > with "-m32" which might cause ICE. > > So MOVE_MAX_PIECES and COMPARE_MAX_PIECES is set to 4 for 32 bit > target in this patch. They could be changed to 16 after rs6000 > supports TImode with "-m32". > Hi, Hao Thanks for the explanation. Are you stating that although PPC32 supports V16QImode in VSX, the move_by_pieces support also requires TImode, which is not available on PPC32? Thanks, David
Re: [PATCH-2, rs6000] Enable vector mode for memory equality compare [PR111449]
On Sun, Oct 8, 2023 at 10:30 PM HAO CHEN GUI wrote:
> Hi,
> This patch enables vector mode for memory equality compare by adding
> a new expand cbranchv16qi4 and implementing it. Also the corresponding
> CC reg and compare code is set in rs6000_generate_compare. With the
> patch, 16-byte equality compare can be implemented by one vector compare
> instructions other than 2 8-byte compares with branches.
>
> The test case is in the second patch which is rs6000 specific.
>
> Bootstrapped and tested on powerpc64-linux BE and LE with no
> regressions.
>
Thanks for working on this.
>
> Thanks
> Gui Haochen
>
> ChangeLog
> rs6000: Enable vector compare for memory equality compare
>
> gcc/
> PR target/111449
> * config/rs6000/altivec.md (cbranchv16qi4): New expand pattern.
> * config/rs6000/rs6000.cc (rs6000_generate_compare): Generate insn
> sequence for V16QImode equality compare.
> * config/rs6000/rs6000.h (MOVE_MAX_PIECES): Define.
> (COMPARE_MAX_PIECES): Define.
>
> gcc/testsuite/
> PR target/111449
> * gcc.target/powerpc/pr111449.c: New.
>
> patch.diff
> diff --git a/gcc/config/rs6000/altivec.md b/gcc/config/rs6000/altivec.md
> index e8a596fb7e9..c69bf266402 100644
> --- a/gcc/config/rs6000/altivec.md
> +++ b/gcc/config/rs6000/altivec.md
> @@ -2605,6 +2605,39 @@ (define_insn "altivec_vupklpx"
> }
>[(set_attr "type" "vecperm")])
>
> +(define_expand "cbranchv16qi4"
> + [(use (match_operator 0 "equality_operator"
> + [(match_operand:V16QI 1 "gpc_reg_operand")
> +(match_operand:V16QI 2 "gpc_reg_operand")]))
> + (use (match_operand 3))]
> + "VECTOR_UNIT_ALTIVEC_P (V16QImode)"
> +{
> + if (!TARGET_P9_VECTOR
> + && MEM_P (operands[1])
> + && !altivec_indexed_or_indirect_operand (operands[1], V16QImode)
> + && MEM_P (operands[2])
> + && !altivec_indexed_or_indirect_operand (operands[2], V16QImode))
> +{
> + /* Use direct move as the byte order doesn't matter for equality
> +compare. */
> + rtx reg_op1 = gen_reg_rtx (V16QImode);
> + rtx reg_op2 = gen_reg_rtx (V16QImode);
> + rs6000_emit_le_vsx_permute (reg_op1, operands[1], V16QImode);
> + rs6000_emit_le_vsx_permute (reg_op2, operands[2], V16QImode);
> + operands[1] = reg_op1;
> + operands[2] = reg_op2;
> +}
> + else
> +{
> + operands[1] = force_reg (V16QImode, operands[1]);
> + operands[2] = force_reg (V16QImode, operands[2]);
> +}
> + rtx_code code = GET_CODE (operands[0]);
> + operands[0] = gen_rtx_fmt_ee (code, V16QImode, operands[1],
> operands[2]);
> + rs6000_emit_cbranch (V16QImode, operands);
> + DONE;
> +})
> +
> ;; Compare vectors producing a vector result and a predicate, setting CR6
> to
> ;; indicate a combined status
> (define_insn "altivec_vcmpequ_p"
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index efe9adce1f8..0087d786840 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -15264,6 +15264,15 @@ rs6000_generate_compare (rtx cmp, machine_mode
> mode)
> else
> emit_insn (gen_stack_protect_testsi (compare_result, op0,
> op1b));
> }
> + else if (mode == V16QImode)
> + {
> + gcc_assert (code == EQ || code == NE);
> +
> + rtx result_vector = gen_reg_rtx (V16QImode);
> + compare_result = gen_rtx_REG (CCmode, CR6_REGNO);
> + emit_insn (gen_altivec_vcmpequb_p (result_vector, op0, op1));
> + code = (code == NE) ? GE : LT;
> + }
>else
> emit_insn (gen_rtx_SET (compare_result,
> gen_rtx_COMPARE (comp_mode, op0, op1)));
> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
> index 3503614efbd..dc33bca0802 100644
> --- a/gcc/config/rs6000/rs6000.h
> +++ b/gcc/config/rs6000/rs6000.h
> @@ -1730,6 +1730,8 @@ typedef struct rs6000_args
> in one reasonably fast instruction. */
> #define MOVE_MAX (! TARGET_POWERPC64 ? 4 : 8)
> #define MAX_MOVE_MAX 8
> +#define MOVE_MAX_PIECES (!TARGET_POWERPC64 ? 4 : 16)
> +#define COMPARE_MAX_PIECES (!TARGET_POWERPC64 ? 4 : 16)
>
How are the definitions of MOVE_MAX_PIECES and COMPARE_MAX_PIECES
determined? The email does not provide any explanation for the
implementation. The rest of the patch is related to vector support, but
vector support is not dependent on TARGET_POWERPC64.
Thanks, David
>
> /* Nonzero if access to memory by bytes is no faster than for words.
> Also nonzero if doing byte operations (specifically shifts) in
> registers
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr111449.c
> b/gcc/testsuite/gcc.target/powerpc/pr111449.c
> new file mode 100644
> index 000..a8c30b92a41
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr111449.c
> @@ -0,0 +1,19 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_p8vector_ok } */
> +/* { dg-options "-maltivec -O2" } */
>
Re: [PATCH V5 2/2] rs6000: use mtvsrws to move sf from si p9
On Thu, Oct 5, 2023 at 12:14 AM Jiufu Guo wrote:
> Hi,
>
> As mentioned in PR108338, on p9, we could use mtvsrws to implement
> the bitcast from SI to SF (or lowpart DI to SF).
>
> For example:
> *(long long*)buff = di;
> float f = *(float*)(buff);
>
> "sldi 9,3,32 ; mtvsrd 1,9 ; xscvspdpn 1,1" is generated.
> A better one would be "mtvsrws 1,3 ; xscvspdpn 1,1".
>
> Compare with previous patch:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628791.html
> According to review comments, this version refines commit message
> and words in comments, also updates the test case
>
> Pass bootstrap and regtest on ppc64{,le}.
> Is this ok for trunk?
>
Okay.
Thanks, David
>
> BR,
> Jeff (Jiufu Guo)
>
> PR target/108338
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.md (movsf_from_si): Update to generate
> mtvsrws
> for P9.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/powerpc/pr108338.c: Updated to check mtvsrws for p9.
>
> ---
> gcc/config/rs6000/rs6000.md | 25 -
> gcc/testsuite/gcc.target/powerpc/pr108338.c | 21 ++---
> 2 files changed, 37 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 56bd8bc1147..d6dfb25cea0 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -8283,13 +8283,26 @@ (define_insn_and_split "movsf_from_si"
> {
>rtx op0 = operands[0];
>rtx op1 = operands[1];
> - rtx op2 = operands[2];
> - rtx op1_di = gen_rtx_REG (DImode, REGNO (op1));
>
> - /* Move SF value to upper 32-bits for xscvspdpn. */
> - emit_insn (gen_ashldi3 (op2, op1_di, GEN_INT (32)));
> - emit_insn (gen_p8_mtvsrd_sf (op0, op2));
> - emit_insn (gen_vsx_xscvspdpn_directmove (op0, op0));
> + /* Move lowpart 32-bits from register for SFmode. */
> + if (TARGET_P9_VECTOR)
> +{
> + /* Using mtvsrws;xscvspdpn. */
> + rtx op0_v = gen_rtx_REG (V4SImode, REGNO (op0));
> + emit_insn (gen_vsx_splat_v4si (op0_v, op1));
> + emit_insn (gen_vsx_xscvspdpn_directmove (op0, op0));
> +}
> + else
> +{
> + rtx op2 = operands[2];
> + rtx op1_di = gen_rtx_REG (DImode, REGNO (op1));
> +
> + /* Using sldi;mtvsrd;xscvspdpn. */
> + emit_insn (gen_ashldi3 (op2, op1_di, GEN_INT (32)));
> + emit_insn (gen_p8_mtvsrd_sf (op0, op2));
> + emit_insn (gen_vsx_xscvspdpn_directmove (op0, op0));
> +}
> +
>DONE;
> }
>[(set_attr "length"
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr108338.c
> b/gcc/testsuite/gcc.target/powerpc/pr108338.c
> index bd83c0b3ad8..5f2f62866ee 100644
> --- a/gcc/testsuite/gcc.target/powerpc/pr108338.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr108338.c
> @@ -3,9 +3,12 @@
> /* { dg-options "-O2 -save-temps" } */
>
> /* Under lp64, parameter 'v' is in DI regs, then bitcast sub DI to SF. */
> -/* { dg-final { scan-assembler-times {\mxscvspdpn\M} 1 { target { lp64 &&
> has_arch_pwr8 } } } } */
> -/* { dg-final { scan-assembler-times {\mmtvsrd\M} 1 { target { lp64 &&
> has_arch_pwr8 } } } } */
> +/* { dg-final { scan-assembler-times {\mxscvspdpn\M} 2 { target { lp64 &&
> has_arch_pwr8 } } } } */
> +/* { dg-final { scan-assembler-times {\mmtvsrd\M} 2 { target { lp64 && {
> has_arch_pwr8 && { ! has_arch_pwr9 } } } } } } */
> +/* { dg-final { scan-assembler-times {\mmtvsrd\M} 1 { target { lp64 &&
> has_arch_pwr9 } } } } */
> +/* { dg-final { scan-assembler-times {\mmtvsrws\M} 1 { target { lp64 &&
> has_arch_pwr9 } } } } */
> /* { dg-final { scan-assembler-times {\mrldicr\M} 1 { target { lp64 &&
> has_arch_pwr8 } } } } */
> +/* { dg-final { scan-assembler-times {\msldi\M} 1 { target { lp64 && {
> has_arch_pwr8 && { ! has_arch_pwr9 } } } } } } */
>
> struct di_sf_sf
> {
> @@ -22,16 +25,28 @@ sf_from_high32bit_di (struct di_sf_sf v)
> #endif
> }
>
> +float __attribute__ ((noipa))
> +sf_from_low32bit_di (struct di_sf_sf v)
> +{
> +#ifdef __LITTLE_ENDIAN__
> + return v.f1;
> +#else
> + return v.f2;
> +#endif
> +}
> +
> int main()
> {
>struct di_sf_sf v;
>v.f1 = v.f2 = 0.0f;
> #ifdef __LITTLE_ENDIAN__
> + v.f1 = 1.0f;
>v.f2 = 2.0f;
> #else
>v.f1 = 2.0f;
> + v.f2 = 1.0f;
> #endif
> - if (sf_from_high32bit_di (v) != 2.0f)
> + if (sf_from_high32bit_di (v) != 2.0f || sf_from_low32bit_di (v) != 1.0f)
> __builtin_abort ();
>return 0;
> }
> --
> 2.25.1
>
>
Re: [PATCH V5 1/2] rs6000: optimize moving to sf from highpart di
On Thu, Oct 5, 2023 at 12:50 AM Jiufu Guo wrote:
> Hi,
>
> Currently, we have the pattern "movsf_from_si2" which was trying
> to support moving high part DI to SF.
>
> But current pattern only accepts "ashiftrt":
> XX:SF=bitcast:SF(subreg(YY:DI>>32),0), but actually "lshiftrt" should
> also be ok.
> And current pattern only supports BE.
>
> This patch updats the pattern to support BE and "lshiftrt".
>
> Compare with previous version:
> https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628790.html
> This version refines the code slightly and updates the test case
> according to review comments.
>
> Pass bootstrap and regtest on ppc64{,le}.
> Is this ok for trunk?
>
Okay.
Thanks, David
>
> BR,
> Jeff (Jiufu Guo)
>
> PR target/108338
>
> gcc/ChangeLog:
>
> * config/rs6000/predicates.md (lowpart_subreg_operator): New
> define_predicate.
> * config/rs6000/rs6000.md (any_rshift): New code_iterator.
> (movsf_from_si2): Rename to ...
> (movsf_from_si2_): ... this.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/powerpc/pr108338.c: New test.
>
> ---
> gcc/config/rs6000/predicates.md | 5 +++
> gcc/config/rs6000/rs6000.md | 12 ---
> gcc/testsuite/gcc.target/powerpc/pr108338.c | 37 +
> 3 files changed, 49 insertions(+), 5 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr108338.c
>
> diff --git a/gcc/config/rs6000/predicates.md
> b/gcc/config/rs6000/predicates.md
> index 925f69cd3fc..ef7d3f214c4 100644
> --- a/gcc/config/rs6000/predicates.md
> +++ b/gcc/config/rs6000/predicates.md
> @@ -2098,3 +2098,8 @@ (define_predicate "macho_pic_address"
>else
> return false;
> })
> +
> +(define_predicate "lowpart_subreg_operator"
> + (and (match_code "subreg")
> + (match_test "subreg_lowpart_offset (mode, GET_MODE (SUBREG_REG
> (op)))
> + == SUBREG_BYTE (op)")))
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 1a9a7b1a479..56bd8bc1147 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -643,6 +643,9 @@ (define_code_iterator any_extend[sign_extend
> zero_extend])
> (define_code_iterator any_fix [fix unsigned_fix])
> (define_code_iterator any_float[float unsigned_float])
>
> +; Shift right.
> +(define_code_iterator any_shiftrt [ashiftrt lshiftrt])
> +
> (define_code_attr u [(sign_extend "")
> (zero_extend "u")
> (fix "")
> @@ -8303,14 +8306,13 @@ (define_insn_and_split "movsf_from_si"
> ;; {%1:SF=unspec[r122:DI>>0x20#0] 86;clobber scratch;}
> ;; split it before reload with "and mask" to avoid generating shift right
> ;; 32 bit then shift left 32 bit.
> -(define_insn_and_split "movsf_from_si2"
> +(define_insn_and_split "movsf_from_si2_"
>[(set (match_operand:SF 0 "gpc_reg_operand" "=wa")
> (unspec:SF
> -[(subreg:SI
> - (ashiftrt:DI
> +[(match_operator:SI 3 "lowpart_subreg_operator"
> + [(any_shiftrt:DI
> (match_operand:DI 1 "input_operand" "r")
> - (const_int 32))
> - 0)]
> + (const_int 32))])]
> UNSPEC_SF_FROM_SI))
>(clobber (match_scratch:DI 2 "=r"))]
>"TARGET_NO_SF_SUBREG"
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr108338.c
> b/gcc/testsuite/gcc.target/powerpc/pr108338.c
> new file mode 100644
> index 000..bd83c0b3ad8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr108338.c
> @@ -0,0 +1,37 @@
> +/* { dg-do run } */
> +/* { dg-require-effective-target hard_float } */
> +/* { dg-options "-O2 -save-temps" } */
> +
> +/* Under lp64, parameter 'v' is in DI regs, then bitcast sub DI to SF. */
> +/* { dg-final { scan-assembler-times {\mxscvspdpn\M} 1 { target { lp64 &&
> has_arch_pwr8 } } } } */
> +/* { dg-final { scan-assembler-times {\mmtvsrd\M} 1 { target { lp64 &&
> has_arch_pwr8 } } } } */
> +/* { dg-final { scan-assembler-times {\mrldicr\M} 1 { target { lp64 &&
> has_arch_pwr8 } } } } */
> +
> +struct di_sf_sf
> +{
> + float f1; float f2; long long l;
> +};
> +
> +float __attribute__ ((noipa))
> +sf_from_high32bit_di (struct di_sf_sf v)
> +{
> +#ifdef __LITTLE_ENDIAN__
> + return v.f2;
> +#else
> + return v.f1;
> +#endif
> +}
> +
> +int main()
> +{
> + struct di_sf_sf v;
> + v.f1 = v.f2 = 0.0f;
> +#ifdef __LITTLE_ENDIAN__
> + v.f2 = 2.0f;
> +#else
> + v.f1 = 2.0f;
> +#endif
> + if (sf_from_high32bit_di (v) != 2.0f)
> +__builtin_abort ();
> + return 0;
> +}
> --
> 2.25.1
>
>
Re: [PATCH v2] Add a GCC Security policy
Hi, Siddhesh Thanks for working on this and fine-tuning the original proposed text. It looks good to me. Minor grammatical nit below. Thanks, David On Thu, Sep 28, 2023 at 7:59 AM Siddhesh Poyarekar wrote: > On 2023-09-28 12:55, Siddhesh Poyarekar wrote: > > Define a security process and exclusions to security issues for GCC and > > all components it ships. > > > > Signed-off-by: Siddhesh Poyarekar > > --- > > Sorry I forgot to summarize changes since the previous version: > > - Reworded the introduction so that it doesn't sound like we know *all* > scenarios and also encourage reporters to reach out. > > - Fixed up support and diagnostic libraries sections based on Jakub's > feedback. > > > SECURITY.txt | 205 +++ > > 1 file changed, 205 insertions(+) > > create mode 100644 SECURITY.txt > > > > diff --git a/SECURITY.txt b/SECURITY.txt > > new file mode 100644 > > index 000..14cb31570d3 > > --- /dev/null > > +++ b/SECURITY.txt > > @@ -0,0 +1,205 @@ > > +What is a GCC security bug? > > +=== > > + > > +A security bug is one that threatens the security of a system or > > +network, or might compromise the security of data stored on it. > > +In the context of GCC there are multiple ways in which this might > > +happen and some common scenarios are detailed below. > > + > > +If you're reporting a security issue and feel like it does not fit > > +into any of the descriptions below, you're encouraged to reach out > > +through the GCC bugzilla or if needed, privately by following the > Some missing, pedantic commas: through the GCC bugzilla or, if needed, privately, by following the > > +instructions in the last two sections of this document. > > + > > +Compiler drivers, programs, libgccjit and support libraries > > +--- > > + > > +The compiler driver processes source code, invokes other programs > > +such as the assembler and linker and generates the output result, > > +which may be assembly code or machine code. Compiling untrusted > > +sources can result in arbitrary code execution and unconstrained > > +resource consumption in the compiler. As a result, compilation of > > +such code should be done inside a sandboxed environment to ensure > > +that it does not compromise the development environment. > > + > > +The libgccjit library can, despite the name, be used both for > > +ahead-of-time compilation and for just-in-compilation. In both > > +cases it can be used to translate input representations (such as > > +source code) in the application context; in the latter case the > > +generated code is also run in the application context. > > + > > +Limitations that apply to the compiler driver, apply here too in > > +terms of sanitizing inputs and it is recommended that both the > > +compilation *and* execution context of the code are appropriately > > +sandboxed to contain the effects of any bugs in libgccjit, the > > +application code using it, or its generated code to the sandboxed > > +environment. > > + > > +Libraries such as libiberty, libcc1 and libcpp are not distributed > > +for runtime support and have similar challenges to compiler drivers. > > +While they are expected to be robust against arbitrary input, they > > +should only be used with trusted inputs when linked into the > > +compiler. > > + > > +Libraries such as zlib that bundled into GCC to build it will be > > +treated the same as the compiler drivers and programs as far as > > +security coverage is concerned. However if you find an issue in > > +these libraries independent of their use in GCC, you should reach > > +out to their upstream projects to report them. > > + > > +As a result, the only case for a potential security issue in the > > +compiler is when it generates vulnerable application code for > > +trusted input source code that is conforming to the relevant > > +programming standard or extensions documented as supported by GCC > > +and the algorithm expressed in the source code does not have the > > +vulnerability. The output application code could be considered > > +vulnerable if it produces an actual vulnerability in the target > > +application, specifically in the following cases: > > + > > +- The application dereferences an invalid memory location despite > > + the application sources being valid. > > +- The application reads from or writes to a valid but incorrect > > + memory location, resulting in an information integrity issue or an > > + information leak. > > +- The application ends up running in an infinite loop or with > > + severe degradation in performance despite the input sources having > > + no such issue, resulting in a Denial of Service. Note that > > +
Re: [COMMITTED] Return TRUE only when a global value is updated.
AIX bootstrap is happier with the patch. Thanks, David On Tue, Oct 3, 2023 at 12:30 PM Andrew MacLeod wrote: > Give this a try.. I'm testing it here, but x86 doesn't seem to show it > anyway for some reason :-P > > I think i needed to handle pointers special since SSA_NAMES handle > pointer ranges different. > > Andrew > > On 10/3/23 11:47, David Edelsohn wrote: > > This patch caused a bootstrap failure on AIX. > > > > during GIMPLE pass: evrp > > > > /nasfarm/edelsohn/src/src/libgcc/libgcc2.c: In function '__gcc_bcmp': > > > > /nasfarm/edelsohn/src/src/libgcc/libgcc2.c:2910:1: internal compiler > > error: in get_irange, at value-range-storage.cc:343 > > > > 2910 | } > > > > | ^ > > > > > > 0x11b7f4b7 irange_storage::get_irange(irange&, tree_node*) const > > > > /nasfarm/edelsohn/src/src/gcc/value-range-storage.cc:343 > > > > 0x11b7e7af vrange_storage::get_vrange(vrange&, tree_node*) const > > > > /nasfarm/edelsohn/src/src/gcc/value-range-storage.cc:178 > > > > 0x139f3d77 range_info_get_range(tree_node const*, vrange&) > > > > /nasfarm/edelsohn/src/src/gcc/tree-ssanames.cc:118 > > > > 0x1134b463 set_range_info(tree_node*, vrange const&) > > > > /nasfarm/edelsohn/src/src/gcc/tree-ssanames.cc:425 > > > > 0x116a7333 gimple_ranger::register_inferred_ranges(gimple*) > > > > /nasfarm/edelsohn/src/src/gcc/gimple-range.cc:487 > > > > 0x125cef27 rvrp_folder::fold_stmt(gimple_stmt_iterator*) > > > > /nasfarm/edelsohn/src/src/gcc/tree-vrp.cc:1033 > > > > 0x123dd063 > > substitute_and_fold_dom_walker::before_dom_children(basic_block_def*) > > > > /nasfarm/edelsohn/src/src/gcc/tree-ssa-propagate.cc:876 > > > > 0x1176cc43 dom_walker::walk(basic_block_def*) > > > > /nasfarm/edelsohn/src/src/gcc/domwalk.cc:311 > > > > 0x123dd733 > > substitute_and_fold_engine::substitute_and_fold(basic_block_def*) > > > > /nasfarm/edelsohn/src/src/gcc/tree-ssa-propagate.cc:999 > > > > 0x123d0f5f execute_ranger_vrp(function*, bool, bool) > > > > /nasfarm/edelsohn/src/src/gcc/tree-vrp.cc:1062 > > > > 0x123d14ef execute > > > > /nasfarm/edelsohn/src/src/gcc/tree-vrp.cc:1142 > >
Re: [COMMITTED] Return TRUE only when a global value is updated.
This patch caused a bootstrap failure on AIX. during GIMPLE pass: evrp /nasfarm/edelsohn/src/src/libgcc/libgcc2.c: In function '__gcc_bcmp': /nasfarm/edelsohn/src/src/libgcc/libgcc2.c:2910:1: internal compiler error: in get_irange, at value-range-storage.cc:343 2910 | } | ^ 0x11b7f4b7 irange_storage::get_irange(irange&, tree_node*) const /nasfarm/edelsohn/src/src/gcc/value-range-storage.cc:343 0x11b7e7af vrange_storage::get_vrange(vrange&, tree_node*) const /nasfarm/edelsohn/src/src/gcc/value-range-storage.cc:178 0x139f3d77 range_info_get_range(tree_node const*, vrange&) /nasfarm/edelsohn/src/src/gcc/tree-ssanames.cc:118 0x1134b463 set_range_info(tree_node*, vrange const&) /nasfarm/edelsohn/src/src/gcc/tree-ssanames.cc:425 0x116a7333 gimple_ranger::register_inferred_ranges(gimple*) /nasfarm/edelsohn/src/src/gcc/gimple-range.cc:487 0x125cef27 rvrp_folder::fold_stmt(gimple_stmt_iterator*) /nasfarm/edelsohn/src/src/gcc/tree-vrp.cc:1033 0x123dd063 substitute_and_fold_dom_walker::before_dom_children(basic_block_def*) /nasfarm/edelsohn/src/src/gcc/tree-ssa-propagate.cc:876 0x1176cc43 dom_walker::walk(basic_block_def*) /nasfarm/edelsohn/src/src/gcc/domwalk.cc:311 0x123dd733 substitute_and_fold_engine::substitute_and_fold(basic_block_def*) /nasfarm/edelsohn/src/src/gcc/tree-ssa-propagate.cc:999 0x123d0f5f execute_ranger_vrp(function*, bool, bool) /nasfarm/edelsohn/src/src/gcc/tree-vrp.cc:1062 0x123d14ef execute /nasfarm/edelsohn/src/src/gcc/tree-vrp.cc:1142
Re: [PATCH] rs6000: Make 32 bit stack_protect support prefixed insn [PR111367]
On Wed, Sep 27, 2023 at 1:38 AM Kewen.Lin wrote:
> Hi,
>
> As PR111367 shows, with prefixed insn supported, some of
> checkings consider it's able to leverage prefixed insn
> for stack protect related load/store, but since we don't
> actually change the emitted assembly for 32 bit, it can
> cause the assembler error as exposed.
>
> Mike's commit r10-4547-gce6a6c007e5a98 has already handled
> the 64 bit case (DImode), this patch is to treat the 32
> bit case (SImode) by making use of mode iterator P and
> ptrload attribute iterator, also fixes the constraints
> to match the emitted operand formats.
>
> Bootstrapped and regtested on powerpc64-linux-gnu P7/P8/P9
> and powerpc64le-linux-gnu P9.
>
> This patch has incorporated Segher's comments in PR111367,
> I'm going to push this soon if no objections.
>
This patch is okay.
Thanks, David
>
> BR,
> Kewen
> -
> PR target/111367
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.md (stack_protect_setsi): Support prefixed
> instruction emission and incorporate to stack_protect_set.
> (stack_protect_setdi): Rename to ...
> (stack_protect_set): ... this, adjust constraint.
> (stack_protect_testsi): Support prefixed instruction emission and
> incorporate to stack_protect_test.
> (stack_protect_testdi): Rename to ...
> (stack_protect_test): ... this, adjust constraint.
>
> gcc/testsuite/ChangeLog:
>
> * g++.target/powerpc/pr111367.C: New test.
> ---
> gcc/config/rs6000/rs6000.md | 73 -
> gcc/testsuite/g++.target/powerpc/pr111367.C | 22 +++
> 2 files changed, 49 insertions(+), 46 deletions(-)
> create mode 100644 gcc/testsuite/g++.target/powerpc/pr111367.C
>
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 1a9a7b1a479..0ac79fc7735 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -12389,33 +12389,26 @@ (define_expand "stack_protect_set"
>DONE;
> })
>
> -(define_insn "stack_protect_setsi"
> - [(set (match_operand:SI 0 "memory_operand" "=m")
> - (unspec:SI [(match_operand:SI 1 "memory_operand" "m")]
> UNSPEC_SP_SET))
> - (set (match_scratch:SI 2 "=") (const_int 0))]
> - "TARGET_32BIT"
> - "lwz%U1%X1 %2,%1\;stw%U0%X0 %2,%0\;li %2,0"
> - [(set_attr "type" "three")
> - (set_attr "length" "12")])
> -
> ;; We can't use the prefixed attribute here because there are two memory
> ;; instructions. We can't split the insn due to the fact that this
> operation
> ;; needs to be done in one piece.
> -(define_insn "stack_protect_setdi"
> - [(set (match_operand:DI 0 "memory_operand" "=Y")
> - (unspec:DI [(match_operand:DI 1 "memory_operand" "Y")]
> UNSPEC_SP_SET))
> - (set (match_scratch:DI 2 "=") (const_int 0))]
> - "TARGET_64BIT"
> +(define_insn "stack_protect_set"
> + [(set (match_operand:P 0 "memory_operand" "=YZ")
> + (unspec:P [(match_operand:P 1 "memory_operand" "YZ")]
> UNSPEC_SP_SET))
> + (set (match_scratch:P 2 "=") (const_int 0))]
> + ""
> {
> - if (prefixed_memory (operands[1], DImode))
> -output_asm_insn ("pld %2,%1", operands);
> + if (prefixed_memory (operands[1], mode))
> +/* Prefixed load only supports D-form but no update and X-form. */
> +output_asm_insn ("p %2,%1", operands);
>else
> -output_asm_insn ("ld%U1%X1 %2,%1", operands);
> +output_asm_insn ("%U1%X1 %2,%1", operands);
>
> - if (prefixed_memory (operands[0], DImode))
> -output_asm_insn ("pstd %2,%0", operands);
> + if (prefixed_memory (operands[0], mode))
> +/* Prefixed store only supports D-form but no update and X-form. */
> +output_asm_insn ("pst %2,%0", operands);
>else
> -output_asm_insn ("std%U0%X0 %2,%0", operands);
> +output_asm_insn ("st%U0%X0 %2,%0", operands);
>
>return "li %2,0";
> }
> @@ -12461,45 +12454,33 @@ (define_expand "stack_protect_test"
>DONE;
> })
>
> -(define_insn "stack_protect_testsi"
> - [(set (match_operand:CCEQ 0 "cc_reg_operand" "=x,?y")
> -(unspec:CCEQ [(match_operand:SI 1 "memory_operand" "m,m")
> - (match_operand:SI 2 "memory_operand" "m,m")]
> -UNSPEC_SP_TEST))
> - (set (match_scratch:SI 4 "=r,r") (const_int 0))
> - (clobber (match_scratch:SI 3 "=,"))]
> - "TARGET_32BIT"
> - "@
> - lwz%U1%X1 %3,%1\;lwz%U2%X2 %4,%2\;xor. %3,%3,%4\;li %4,0
> - lwz%U1%X1 %3,%1\;lwz%U2%X2 %4,%2\;cmplw %0,%3,%4\;li %3,0\;li %4,0"
> - [(set_attr "length" "16,20")])
> -
> ;; We can't use the prefixed attribute here because there are two memory
> ;; instructions. We can't split the insn due to the fact that this
> operation
> ;; needs to be done in one piece.
> -(define_insn "stack_protect_testdi"
> +(define_insn "stack_protect_test"
>[(set (match_operand:CCEQ 0 "cc_reg_operand" "=x,?y")
> -(unspec:CCEQ [(match_operand:DI 1 "memory_operand" "Y,Y")
> - (match_operand:DI 2 "memory_operand" "Y,Y")]
> +
Re: [PATCH v3] RISC-V:Optimize the MASK opt generation
The patch works on AIX. I have Gawk installed, but it is a very old release before multi-dimensional array support was added. Thanks, David On Mon, Oct 2, 2023 at 10:38 PM Kito Cheng wrote: > Proposed fix, and verified with "mawk" and "gawk -P" (gawk with posix > mode) on my linux also some other report it work on freebsd, just wait > review :) > > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/631785.html > > On Tue, Oct 3, 2023 at 2:07 AM Jeff Law wrote: > > > > > > > > On 10/2/23 12:03, David Edelsohn wrote: > > > On Mon, Oct 2, 2023 at 1:59 PM Jeff Law > > <mailto:jeffreya...@gmail.com>> wrote: > > > > > > > > > > > > On 10/2/23 11:20, David Edelsohn wrote: > > > > Wang, > > > > > > > > The AWK portions of this patch broke bootstrap on AIX. > > > > > > > > Also, the AWK portions are common code, not RISC-V specific. I > > > don't > > > > see anywhere that the common portions of the patch were > reviewed or > > > > approved by anyone with authority to approve the changes to the > > > AWK files. > > > > > > > > This patch should not have been committed without approval by a > > > reviewer > > > > with authority for that portion of the compiler and should have > been > > > > tested on targets other than RISC-V if common parts of the > > > compiler were > > > > changed. > > > I acked the generic bits. So the lack of testing on another > target is > > > on me. > > > > > > > > > Hi, Jeff > > > > > > Sorry. I didn't see a comment from a global reviewer in the V3 thread. > > NP. > > > > > > > > I am using Gawk on AIX. After the change, I see a parse error from > > > gawk. I'm rebuilding with a checkout just before the change to confirm > > > that it was the source of the error, and it seems to be past that > > > failure location. I didn't keep the exact error. Once I get past this > > > build cycle, I'll reproduce it. > > I think there's already a patch circulating which fixes this. It broke > > at least one other platform. Hopefully it'll all be sorted out today. > > > > > > jeff >
Re: [PATCH v3] RISC-V:Optimize the MASK opt generation
On Mon, Oct 2, 2023 at 1:59 PM Jeff Law wrote: > > > On 10/2/23 11:20, David Edelsohn wrote: > > Wang, > > > > The AWK portions of this patch broke bootstrap on AIX. > > > > Also, the AWK portions are common code, not RISC-V specific. I don't > > see anywhere that the common portions of the patch were reviewed or > > approved by anyone with authority to approve the changes to the AWK > files. > > > > This patch should not have been committed without approval by a reviewer > > with authority for that portion of the compiler and should have been > > tested on targets other than RISC-V if common parts of the compiler were > > changed. > I acked the generic bits. So the lack of testing on another target is > on me. > Hi, Jeff Sorry. I didn't see a comment from a global reviewer in the V3 thread. I am using Gawk on AIX. After the change, I see a parse error from gawk. I'm rebuilding with a checkout just before the change to confirm that it was the source of the error, and it seems to be past that failure location. I didn't keep the exact error. Once I get past this build cycle, I'll reproduce it. Thanks, David
Re: [PATCH v3] RISC-V:Optimize the MASK opt generation
Wang, The AWK portions of this patch broke bootstrap on AIX. Also, the AWK portions are common code, not RISC-V specific. I don't see anywhere that the common portions of the patch were reviewed or approved by anyone with authority to approve the changes to the AWK files. This patch should not have been committed without approval by a reviewer with authority for that portion of the compiler and should have been tested on targets other than RISC-V if common parts of the compiler were changed. Thanks, David
Re: Ping^2 [PATCH V5 1/4] rs6000: build constant via li;rotldi
On Mon, Sep 25, 2023 at 10:52 PM Jiufu Guo wrote:
> Hi,
>
> Gentle ping...
>
> BR,
> Jeff (Jiufu Guo)
>
This is okay.
Thanks, David
>
> Jiufu Guo via Gcc-patches writes:
>
> > Hi,
> >
> > Gentle ping...
> >
> > BR,
> > Jeff (Jiufu Guo)
> >
> > Jiufu Guo writes:
> >
> >> Hi,
> >>
> >> If a constant is possible to be rotated to/from a positive or negative
> >> value which "li" can generated, then "li;rotldi" can be used to build
> >> the constant.
> >>
> >> Compare with the previous version:
> >> https://gcc.gnu.org/pipermail/gcc-patches/2023-July/623528.html
> >> This patch just did minor changes to the comments according to previous
> >> review.
> >>
> >> Bootstrap and regtest pass on ppc64{,le}.
> >>
> >> Is this ok for trunk?
> >>
> >>
> >> BR,
> >> Jeff (Jiufu)
> >>
> >> gcc/ChangeLog:
> >>
> >> * config/rs6000/rs6000.cc (can_be_built_by_li_and_rotldi): New
> function.
> >> (rs6000_emit_set_long_const): Call can_be_built_by_li_and_rotldi.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> * gcc.target/powerpc/const-build.c: New test.
> >> ---
> >> gcc/config/rs6000/rs6000.cc | 47 +--
> >> .../gcc.target/powerpc/const-build.c | 57 +++
> >> 2 files changed, 98 insertions(+), 6 deletions(-)
> >> create mode 100644 gcc/testsuite/gcc.target/powerpc/const-build.c
> >>
> >> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> >> index 42f49e4a56b..acc332acc05 100644
> >> --- a/gcc/config/rs6000/rs6000.cc
> >> +++ b/gcc/config/rs6000/rs6000.cc
> >> @@ -10258,6 +10258,31 @@ rs6000_emit_set_const (rtx dest, rtx source)
> >>return true;
> >> }
> >>
> >> +/* Check if value C can be built by 2 instructions: one is 'li',
> another is
> >> + 'rotldi'.
> >> +
> >> + If so, *SHIFT is set to the shift operand of rotldi(rldicl), and
> *MASK
> >> + is set to the mask operand of rotldi(rldicl), and return true.
> >> + Return false otherwise. */
> >> +
> >> +static bool
> >> +can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
> >> + HOST_WIDE_INT *mask)
> >> +{
> >> + /* If C or ~C contains at least 49 successive zeros, then C can be
> rotated
> >> + to/from a positive or negative value that 'li' is able to load.
> */
> >> + int n;
> >> + if (can_be_rotated_to_lowbits (c, 15, )
> >> + || can_be_rotated_to_lowbits (~c, 15, ))
> >> +{
> >> + *mask = HOST_WIDE_INT_M1;
> >> + *shift = HOST_BITS_PER_WIDE_INT - n;
> >> + return true;
> >> +}
> >> +
> >> + return false;
> >> +}
> >> +
> >> /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
> >> Output insns to set DEST equal to the constant C as a series of
> >> lis, ori and shl instructions. */
> >> @@ -10266,15 +10291,14 @@ static void
> >> rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
> >> {
> >>rtx temp;
> >> + int shift;
> >> + HOST_WIDE_INT mask;
> >>HOST_WIDE_INT ud1, ud2, ud3, ud4;
> >>
> >>ud1 = c & 0x;
> >> - c = c >> 16;
> >> - ud2 = c & 0x;
> >> - c = c >> 16;
> >> - ud3 = c & 0x;
> >> - c = c >> 16;
> >> - ud4 = c & 0x;
> >> + ud2 = (c >> 16) & 0x;
> >> + ud3 = (c >> 32) & 0x;
> >> + ud4 = (c >> 48) & 0x;
> >>
> >>if ((ud4 == 0x && ud3 == 0x && ud2 == 0x && (ud1 &
> 0x8000))
> >>|| (ud4 == 0 && ud3 == 0 && ud2 == 0 && ! (ud1 & 0x8000)))
> >> @@ -10305,6 +10329,17 @@ rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)
> >>emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
> >> GEN_INT ((ud2 ^ 0x) << 16)));
> >> }
> >> + else if (can_be_built_by_li_and_rotldi (c, , ))
> >> +{
> >> + temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
> >> + unsigned HOST_WIDE_INT imm = (c | ~mask);
> >> + imm = (imm >> shift) | (imm << (HOST_BITS_PER_WIDE_INT - shift));
> >> +
> >> + emit_move_insn (temp, GEN_INT (imm));
> >> + if (shift != 0)
> >> +temp = gen_rtx_ROTATE (DImode, temp, GEN_INT (shift));
> >> + emit_move_insn (dest, temp);
> >> +}
> >>else if (ud3 == 0 && ud4 == 0)
> >> {
> >>temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
> >> diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c
> b/gcc/testsuite/gcc.target/powerpc/const-build.c
> >> new file mode 100644
> >> index 000..69b37e2bb53
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
> >> @@ -0,1 +1,57 @@
> >> +/* { dg-do run } */
> >> +/* { dg-options "-O2 -save-temps" } */
> >> +/* { dg-require-effective-target has_arch_ppc64 } */
> >> +
> >> +/* Verify that two instructions are successfully used to build
> constants.
> >> + One insn is li, another is rotate: rldicl. */
> >> +
> >> +#define NOIPA __attribute__ ((noipa))
> >> +
> >> +struct fun
> >> +{
> >> + long long (*f) (void);
> >> + long long val;
> >> +};
> >> +
> >> +long long NOIPA
> >>
Re: [PATCH] Cleanup: Replace UNSPEC_COPYSIGN with copysign RTL
On Fri, Sep 29, 2023 at 2:09 PM Michael Meissner
wrote:
> When I first implemented COPYSIGN support in the power7 days, we did not
> have a
> copysign RTL insn, so I had to use UNSPEC to represent the copysign
> instruction. This patch removes those UNSPECs, and it uses the native RTL
> copysign insn.
>
> I have tested this on both big endian and little endian PowerPC server
> systems,
> and there were no regressions. Can I check this into the master branch?
> Since
> it is just a clean-up, I don't see the need to back port it, but it is
> simple
> to do the back port if desired.
>
> 2023-09-29 Michael Meissner
>
> gcc/
>
> * config/rs6000/rs6000.md (UNSPEC_COPYSIGN): Delete.
> (copysign3_fcpsg): Use copysign RTL instead of UNSPEC.
> (copysign3_hard): Likewise.
> (copysign3_soft): Likewise.
> * config/rs6000/vector.md (vector_copysign3): Use copysign
> RTL
> instead of UNSPEC.
> * config/rs6000/vsx.md (vsx_copysign3): Use copysign RTL
> instead
> of UNSPEC.
> ---
> gcc/config/rs6000/rs6000.md | 20
> gcc/config/rs6000/vector.md | 4 ++--
> gcc/config/rs6000/vsx.md| 7 +++
> 3 files changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 7b583d7a69a..1b6b6cb5bbe 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -108,7 +108,6 @@ (define_c_enum "unspec"
> UNSPEC_TOCREL
> UNSPEC_MACHOPIC_OFFSET
> UNSPEC_BPERM
> - UNSPEC_COPYSIGN
> UNSPEC_PARITY
> UNSPEC_CMPB
> UNSPEC_FCTIW
> @@ -5383,9 +5382,8 @@ (define_expand "copysign3"
> ;; compiler from optimizing -0.0
>
The comment above the define_insn refers to UNSPEC instead of if-then-else
because of -0.0.
Please remove or update the comment because the pattern no longer uses
UNSPEC.
The rest of the patch is okay with that change.
Thanks, David
> (define_insn "copysign3_fcpsgn"
>[(set (match_operand:SFDF 0 "gpc_reg_operand" "=d,wa")
> - (unspec:SFDF [(match_operand:SFDF 1 "gpc_reg_operand" "d,wa")
> - (match_operand:SFDF 2 "gpc_reg_operand" "d,wa")]
> -UNSPEC_COPYSIGN))]
> + (copysign:SFDF (match_operand:SFDF 1 "gpc_reg_operand" "d,wa")
> + (match_operand:SFDF 2 "gpc_reg_operand" "d,wa")))]
>"TARGET_HARD_FLOAT && (TARGET_CMPB || VECTOR_UNIT_VSX_P (mode))"
>"@
> fcpsgn %0,%2,%1
> @@ -14984,10 +14982,9 @@ (define_expand "copysign3"
>
> (define_insn "copysign3_hard"
>[(set (match_operand:IEEE128 0 "altivec_register_operand" "=v")
> - (unspec:IEEE128
> -[(match_operand:IEEE128 1 "altivec_register_operand" "v")
> - (match_operand:IEEE128 2 "altivec_register_operand" "v")]
> -UNSPEC_COPYSIGN))]
> + (copysign:IEEE128
> +(match_operand:IEEE128 1 "altivec_register_operand" "v")
> +(match_operand:IEEE128 2 "altivec_register_operand" "v")))]
>"TARGET_FLOAT128_HW && FLOAT128_IEEE_P (mode)"
> "xscpsgnqp %0,%2,%1"
>[(set_attr "type" "vecmove")
> @@ -14995,10 +14992,9 @@ (define_insn "copysign3_hard"
>
> (define_insn "copysign3_soft"
>[(set (match_operand:IEEE128 0 "altivec_register_operand" "=v")
> - (unspec:IEEE128
> -[(match_operand:IEEE128 1 "altivec_register_operand" "v")
> - (match_operand:IEEE128 2 "altivec_register_operand" "v")]
> -UNSPEC_COPYSIGN))
> + (copysign:IEEE128
> +(match_operand:IEEE128 1 "altivec_register_operand" "v")
> +(match_operand:IEEE128 2 "altivec_register_operand" "v")))
> (clobber (match_scratch:IEEE128 3 "="))]
>"!TARGET_FLOAT128_HW && FLOAT128_IEEE_P (mode)"
> "xscpsgndp %x3,%x2,%x1\;xxpermdi %x0,%x3,%x1,1"
> diff --git a/gcc/config/rs6000/vector.md b/gcc/config/rs6000/vector.md
> index 1ae04c8e0a8..f4fc620b653 100644
> --- a/gcc/config/rs6000/vector.md
> +++ b/gcc/config/rs6000/vector.md
> @@ -332,8 +332,8 @@ (define_expand "vector_btrunc2"
>
> (define_expand "vector_copysign3"
>[(set (match_operand:VEC_F 0 "vfloat_operand")
> - (unspec:VEC_F [(match_operand:VEC_F 1 "vfloat_operand")
> - (match_operand:VEC_F 2 "vfloat_operand")]
> UNSPEC_COPYSIGN))]
> + (copysign:VEC_F (match_operand:VEC_F 1 "vfloat_operand")
> + (match_operand:VEC_F 2 "vfloat_operand")))]
>"VECTOR_UNIT_ALTIVEC_OR_VSX_P (mode)"
> {
>if (mode == V4SFmode && VECTOR_UNIT_ALTIVEC_P (mode))
> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
> index 4de41e78d51..f3b40229094 100644
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -2233,10 +2233,9 @@ (define_insn "*vsx_ge__p"
> ;; Copy sign
> (define_insn "vsx_copysign3"
>[(set (match_operand:VSX_F 0 "vsx_register_operand" "=wa")
> - (unspec:VSX_F
> -[(match_operand:VSX_F 1 "vsx_register_operand" "wa")
> - (match_operand:VSX_F 2
Re: [PATCH v2 1/2] libstdc++: Implement more maintainable header
Was the dependency added to the dependencies in contrib/gcc_update?
Otherwise the timestamp can get out of sync in a Git checkout.
Thanks, David
On Wed, Aug 16, 2023 at 6:20 PM Jonathan Wakely wrote:
> On Wed, 16 Aug 2023 at 22:56, Jonathan Wakely wrote:
> >
> > On Wed, 16 Aug 2023 at 22:39, David Edelsohn wrote:
> > >
> > > Hi, Arsen
> > >
> > > This patch broke bootstrap because it has introduced a new GCC build
> requirement for autogen that is not a previous requirement to build GCC.
> Previously the repository has included post-processed files.
> >
> > The repo does include the generated bits/version.h file. autogen
> > should only be needed if you modify version.dep
>
> And I've just checked again with an x86_64-pc-linux-gnu bootstrap on a
> box without autogen, and it worked.
>
> >
> > >
> > > +# AutoGen .
> > > +.PHONY: update-version
> > > +update-version:
> > > + cd ${bits_srcdir} && \
> > > + autogen version.def
> > > +
> > >
> > >
> > > Thanks, David
> > >
> > >
>
>
Re: [PATCH v2 1/2] libstdc++: Implement more maintainable header
Hi, Arsen
This patch broke bootstrap because it has introduced a new GCC build
requirement for autogen that is not a previous requirement to build GCC.
Previously the repository has included post-processed files.
+# AutoGen .
+.PHONY: update-version
+update-version:
+ cd ${bits_srcdir} && \
+ autogen version.def
+
Thanks, David
Re: [RFC] GCC Security policy
On Tue, Aug 15, 2023 at 7:07 PM Alexander Monakov wrote: > > On Tue, 15 Aug 2023, Siddhesh Poyarekar wrote: > > > > Thanks, this is nicer (see notes below). My main concern is that we > > > shouldn't pretend there's some method of verifying that arbitrary > source > > > code is "safe" to pass to an unsandboxed compiler, nor should we push > > > the responsibility of doing that on users. > > > > But responsibility would be pushed to users, wouldn't it? > > Making users responsible for verifying that sources are "safe" is not okay > (we cannot teach them how to do that since there's no general method). > Making users responsible for sandboxing the compiler is fine (there's > a range of sandboxing solutions, from which they can choose according > to their requirements and threat model). Sorry about the ambiguity. > Alex. The compiler should faithfully implement the algorithms described by the programmer. The compiler is responsible if it generates incorrect code for a well-defined, language-conforming program. The compiler cannot be responsible for security issues inherent in the user code, whether that causes the compiler to function in a manner that deteriorates adversely affects the system or generates code that behaves in a manner that adversely affects the system. If "safe" is the wrong word. What word would you suggest? > > So: > > > > The compiler driver processes source code, invokes other programs such > as the > > assembler and linker and generates the output result, which may be > assembly > > code or machine code. Compiling untrusted sources can result in > arbitrary > > code execution and unconstrained resource consumption in the compiler. > As a > > result, compilation of such code should be done inside a sandboxed > environment > > to ensure that it does not compromise the development environment. > > I'm happy with this, thanks for bearing with me. > > > >> inside a sandboxed environment to ensure that it does not compromise > the > > >> development environment. Note that this still does not guarantee > safety of > > >> the produced output programs and that such programs should still > either be > > >> analyzed thoroughly for safety or run only inside a sandbox or an > isolated > > >> system to avoid compromising the execution environment. > > > > > > The last statement seems to be a new addition. It is too broad and > again > > > makes a reference to analysis that appears quite theoretical. It might > be > > > better to drop this (and instead talk in more specific terms about any > > > guarantees that produced binary code matches security properties > intended > > > by the sources; I believe Richard Sandiford raised this previously). > > > > OK, so I actually cover this at the end of the section; Richard's point > AFAICT > > was about hardening, which I added another note for to make it explicit > that > > missed hardening does not constitute a CVE-worthy threat: > > Thanks for the reminder. To illustrate what I was talking about, let me > give > two examples: > > 1) safety w.r.t timing attacks: even if the source code is written in > a manner that looks timing-safe, it might be transformed in a way that > mounting a timing attack on the resulting machine code is possible; > > 2) safety w.r.t information leaks: even if the source code attempts > to discard sensitive data (such as passwords and keys) immediately > after use, (partial) copies of that data may be left on stack and > in registers, to be leaked later via a different vulnerability. > > For both 1) and 2), GCC is not engineered to respect such properties > during optimization and code generation, so it's not appropriate for such > tasks (a possible solution is to isolate such sensitive functions to > separate files, compile to assembly, inspect the assembly to check that it > still has the required properties, and use the inspected asm in subsequent > builds instead of the original high-level source). > At some point the system tools need to respect the programmer or operator. There is a difference between writing "Hello, World" and writing performance critical or safety critical code. That is the responsibility of the programmer and the development team to choose the right software engineers and right tools. And to have the development environment and checks in place to ensure that the results are meeting the requirements. It is not the role of GCC or its security policy to tell people how to do their job or hobby. This isn't a safety tag required to be attached to a new mattress. Thanks, David > > Cheers. > Alexander >
Re: [RFC] GCC Security policy
On Wed, Aug 9, 2023 at 1:33 PM Siddhesh Poyarekar wrote: > On 2023-08-08 10:30, Siddhesh Poyarekar wrote: > >> Do you have a suggestion for the language to address libgcc, > >> libstdc++, etc. and libiberty, libbacktrace, etc.? > > > > I'll work on this a bit and share a draft. > > Hi David, > > Here's what I came up with for different parts of GCC, including the > runtime libraries. Over time we may find that specific parts of runtime > libraries simply cannot be used safely in some contexts and flag that. > > Sid > > """ > What is a GCC security bug? > === > > A security bug is one that threatens the security of a system or > network, or might compromise the security of data stored on it. > In the context of GCC there are multiple ways in which this might > happen and they're detailed below. > > Compiler drivers, programs, libgccjit and support libraries > --- > > The compiler driver processes source code, invokes other programs > such as the assembler and linker and generates the output result, > which may be assembly code or machine code. It is necessary that > all source code inputs to the compiler are trusted, since it is > impossible for the driver to validate input source code beyond > conformance to a programming language standard. > > The GCC JIT implementation, libgccjit, is intended to be plugged > into applications to translate input source code in the application > context. Limitations that apply to the compiler > driver, apply here too in terms of sanitizing inputs, so it is > recommended that inputs are either sanitized by an external program > to allow only trusted, safe execution in the context of the > application or the JIT execution context is appropriately sandboxed > to contain the effects of any bugs in the JIT or its generated code > to the sandboxed environment. > > Support libraries such as libiberty, libcc1 libvtv and libcpp have > been developed separately to share code with other tools such as > binutils and gdb. These libraries again have similar challenges to > compiler drivers. While they are expected to be robust against > arbitrary input, they should only be used with trusted inputs. > > Libraries such as zlib and libffi that bundled into GCC to build it > will be treated the same as the compiler drivers and programs as far > as security coverage is concerned. > > As a result, the only case for a potential security issue in all > these cases is when it ends up generating vulnerable output for > valid input source code. > > Language runtime libraries > -- > > GCC also builds and distributes libraries that are intended to be > used widely to implement runtime support for various programming > languages. These include the following: > > * libada > * libatomic > * libbacktrace > * libcc1 > * libcody > * libcpp > * libdecnumber > * libgcc > * libgfortran > * libgm2 > * libgo > * libgomp > * libiberty > * libitm > * libobjc > * libphobos > * libquadmath > * libssp > * libstdc++ > > These libraries are intended to be used in arbitrary contexts and as > a result, bugs in these libraries may be evaluated for security > impact. However, some of these libraries, e.g. libgo, libphobos, > etc. are not maintained in the GCC project, due to which the GCC > project may not be the correct point of contact for them. You are > encouraged to look at README files within those library directories > to locate the canonical security contact point for those projects. > Hi, Sid The text above states "bugs in these libraries may be evaluated for security impact", but there is no comment about the criteria for a security impact, unlike the GLIBC SECURITY.md document. The text seems to imply the "What is a security bug?" definitions from GLIBC, but the definitions are not explicitly stated in the GCC Security policy. Should this "Language runtime libraries" section include some of the GLIBC "What is a security bug?" text or should the GCC "What is a security bug?" section earlier in this document include the text with a qualification that issues like buffer overflow, memory leaks, information disclosure, etc. specifically apply to "Language runtime libraries" and not all components of GCC? Thanks, David > > Diagnostic libraries > > > The sanitizer library bundled in GCC is intended to be used in > diagnostic cases and not intended for use in sensitive environments. > As a result, bugs in the sanitizer will not be considered security > sensitive. > > GCC plugins > --- > > It should be noted that GCC may execute arbitrary code loaded by a
Re: [RFC] GCC Security policy
On Wed, Aug 9, 2023 at 1:33 PM Siddhesh Poyarekar wrote: > On 2023-08-08 10:30, Siddhesh Poyarekar wrote: > >> Do you have a suggestion for the language to address libgcc, > >> libstdc++, etc. and libiberty, libbacktrace, etc.? > > > > I'll work on this a bit and share a draft. > > Hi David, > > Here's what I came up with for different parts of GCC, including the > runtime libraries. Over time we may find that specific parts of runtime > libraries simply cannot be used safely in some contexts and flag that. > > Sid > Hi, Sid Thanks for iterating on this. > > """ > What is a GCC security bug? > === > > A security bug is one that threatens the security of a system or > network, or might compromise the security of data stored on it. > In the context of GCC there are multiple ways in which this might > happen and they're detailed below. > > Compiler drivers, programs, libgccjit and support libraries > --- > > The compiler driver processes source code, invokes other programs > such as the assembler and linker and generates the output result, > which may be assembly code or machine code. It is necessary that > all source code inputs to the compiler are trusted, since it is > impossible for the driver to validate input source code beyond > conformance to a programming language standard. > > The GCC JIT implementation, libgccjit, is intended to be plugged > into applications to translate input source code in the application > context. Limitations that apply to the compiler > driver, apply here too in terms of sanitizing inputs, so it is > recommended that inputs are either sanitized by an external program > to allow only trusted, safe execution in the context of the > application or the JIT execution context is appropriately sandboxed > to contain the effects of any bugs in the JIT or its generated code > to the sandboxed environment. > > Support libraries such as libiberty, libcc1 libvtv and libcpp have > been developed separately to share code with other tools such as > binutils and gdb. These libraries again have similar challenges to > compiler drivers. While they are expected to be robust against > arbitrary input, they should only be used with trusted inputs. > > Libraries such as zlib and libffi that bundled into GCC to build it > will be treated the same as the compiler drivers and programs as far > as security coverage is concerned. > Should we direct people to the upstream projects for their security policies? > As a result, the only case for a potential security issue in all > these cases is when it ends up generating vulnerable output for > valid input source code. > Language runtime libraries > -- > > GCC also builds and distributes libraries that are intended to be > used widely to implement runtime support for various programming > languages. These include the following: > > * libada > * libatomic > * libbacktrace > * libcc1 > * libcody > * libcpp > * libdecnumber > * libgcc > * libgfortran > * libgm2 > * libgo > * libgomp > * libiberty > * libitm > * libobjc > * libphobos > * libquadmath > * libssp > * libstdc++ > > These libraries are intended to be used in arbitrary contexts and as > a result, bugs in these libraries may be evaluated for security > impact. However, some of these libraries, e.g. libgo, libphobos, > etc. are not maintained in the GCC project, due to which the GCC > project may not be the correct point of contact for them. You are > encouraged to look at README files within those library directories > to locate the canonical security contact point for those projects. > As Richard mentioned, should GCC make a specific statement about the security policy / response for issues that are discovered and fixed in the upstream projects from which the GCC libraries are imported? > > Diagnostic libraries > > > The sanitizer library bundled in GCC is intended to be used in > diagnostic cases and not intended for use in sensitive environments. > As a result, bugs in the sanitizer will not be considered security > sensitive. > > GCC plugins > --- > > It should be noted that GCC may execute arbitrary code loaded by a > user through the GCC plugin mechanism or through system preloading > mechanism. Such custom code should be vetted by the user for safety > as bugs exposed through such code will not be considered security > issues. > Thanks, David
Re: [RFC] GCC Security policy
On Tue, Aug 8, 2023 at 1:36 PM Ian Lance Taylor wrote: > On Tue, Aug 8, 2023 at 7:37 AM Jakub Jelinek wrote: > > > > BTW, I think we should perhaps differentiate between production ready > > libraries (e.g. libgcc, libstdc++, libgomp, libatomic, libgfortran, > libquadmath, > > libssp) vs. e.g. the sanitizer libraries which are meant for debugging > and > > I believe it is highly risky to run them in programs with extra > priviledges > > - e.g. I think they use getenv rather than *secure_getenv to get at > various > > tweaks for their behavior including where logging will happen and > upstream > > doesn't really care. > > And not really sure what to say about lesser used language support > > libraries, libada, libphobos, libgo, libgm2, ... nor what to say about > > libvtv etc. > > libgo is a complicated case because it has a lot of components > including a web server with TLS support, so there are a lot of > potential security issues for programs that use libgo. The upstream > security policy is https://go.dev/security/policy. I'm not sure what > to say about libgo in GCC, since realistically the support for > security problems is best-effort. I guess we should at least accept > security reports, even if we can't promise to fix them quickly. > I believe that upstream projects for components that are imported into GCC should be responsible for their security policy, including libgo, gofrontend, libsanitizer (other than local patches), zlib, libtool, libphobos, libcody, libffi, eventually Rust libcore, etc. Thanks, David
Re: [RFC] GCC Security policy
On Tue, Aug 8, 2023 at 10:07 AM Siddhesh Poyarekar wrote: > On 2023-08-08 10:04, Richard Biener wrote: > > On Tue, Aug 8, 2023 at 3:35 PM Ian Lance Taylor wrote: > >> > >> On Tue, Aug 8, 2023 at 6:02 AM Jakub Jelinek via Gcc-patches > >> wrote: > >>> > >>> On Tue, Aug 08, 2023 at 02:52:57PM +0200, Richard Biener via > Gcc-patches wrote: > There's probably external tools to do this, not sure if we should > replicate > things in the driver for this. > > But sure, I think the driver is the proper point to address any of > such > issues - iff we want to address them at all. Maybe a nice little > google summer-of-code project ;) > >>> > >>> What I'd really like to avoid is having all compiler bugs (primarily > ICEs) > >>> considered to be security bugs (e.g. DoS category), it would be > terrible to > >>> release every week a new compiler because of the "security" issues. > >>> Running compiler on untrusted sources can trigger ICEs (which we want > to fix > >>> but there will always be some), or run into some compile time and/or > compile > >>> memory issue (we have various quadratic or worse spots), compiler stack > >>> limits (deeply nested stuff e.g. during parsing but other areas as > well). > >>> So, people running fuzzers and reporting issues is great, but if > they'd get > >>> a CVE assigned for each ice-on-invalid-code, ice-on-valid-code, > >>> each compile-time-hog and each memory-hog, that wouldn't be useful. > >>> Runtime libraries or security issues in the code we generate for valid > >>> sources are of course a different thing. > >> > >> > >> I wonder if a security policy should say something about the -fplugin > >> option. I agree that an ICE is not a security issue, but I wonder how > >> many people are aware that a poorly chosen command line option can > >> direct the compiler to run arbitrary code. For that matter the same > >> is true of setting the GCC_EXEC_PREFIX environment variable, and no > >> doubt several other environment variables. My point is not that we > >> should change these, but that a security policy should draw attention > >> to the fact that there are cases in which the compiler will > >> unexpectedly run other programs. > > > > Well, if you run an arbitrary commandline from the internet you get > > what you deserve, running "echo "Hello World" | gcc -xc - -o /dev/sda" > > as root doesn't need plugins to shoot yourself in the foot. You need to > > know what you're doing, otherwise you are basically executing an > > arbitrary shell script with whatever privileges you have. > > I think it would be useful to mention caveats with plugins though, just > like it would be useful to mention exceptions for libiberty and similar > libraries that gcc builds. It only helps makes things clearer in terms > of what security coverage the project provides. > I have added a line to the Note section in the proposed text: GCC and its tools provide features and options that can run arbitrary user code (e.g., -fplugin). I believe that the security implication already is addressed because the program is not tricked into a direct compromise of security. Do you have a suggestion for the language to address libgcc, libstdc++, etc. and libiberty, libbacktrace, etc.? Thanks, David
[RFC] GCC Security policy
FOSS Best Practices recommends that projects have an official Security policy stated in a SECURITY.md or SECURITY.txt file at the root of the repository. GLIBC and Binutils have added such documents. Appended is a prototype for a Security policy file for GCC based on the Binutils document because GCC seems to have more affinity with Binutils as a tool. Do the runtime libraries distributed with GCC, especially libgcc, require additional security policies? [ ] Is it appropriate to use the Binutils SECURITY.txt as the starting point or should GCC use GLIBC SECURITY.md as the starting point for the GCC Security policy? [ ] Does GCC, or some components of GCC, require additional care because of runtime libraries like libgcc and libstdc++, and because of gcov and profile-directed feedback? Thoughts? Thanks, David GCC Security Process What is a GCC security bug? === A security bug is one that threatens the security of a system or network, or might compromise the security of data stored on it. In the context of GCC there are two ways in which such bugs might occur. In the first, the programs themselves might be tricked into a direct compromise of security. In the second, the tools might introduce a vulnerability in the generated output that was not already present in the files used as input. Other than that, all other bugs will be treated as non-security issues. This does not mean that they will be ignored, just that they will not be given the priority that is given to security bugs. This stance applies to the creation tools in the GCC (e.g., gcc, g++, gfortran, gccgo, gccrs, gnat, cpp, gcov, etc.) and the libraries that they use. Notes: == None of the programs in GCC need elevated privileges to operate and it is recommended that users do not use them from accounts where such privileges are automatically available. Reporting private security bugs *All bugs reported in the GCC Bugzilla are public.* In order to report a private security bug that is not immediately public, please contact one of the downstream distributions with security teams. The following teams have volunteered to handle such bugs: Debian: secur...@debian.org Red Hat: secal...@redhat.com SUSE:secur...@suse.de Please report the bug to just one of these teams. It will be shared with other teams as necessary. The team contacted will take care of details such as vulnerability rating and CVE assignment (http://cve.mitre.org/about/). It is likely that the team will ask to file a public bug because the issue is sufficiently minor and does not warrant an embargo. An embargo is not a requirement for being credited with the discovery of a security vulnerability. Reporting public security bugs == It is expected that critical security bugs will be rare, and that most security bugs can be reported in GCC, thus making them public immediately. The system can be found here: https://gcc.gnu.org/bugzilla/
Re: [PATCH] match.pd: Implement missed optimization (~X | Y) ^ X -> ~(X & Y) [PR109986]
Hi, Drew Thanks for addressing this missed optimization. The testcase includes an incorrect assumption: signed char, which causes the testcase to fail on PowerPC. Should the testcase be updated to specify signed char in the function signatures or should -fsigned-char be added to the command line options? Thanks, David
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
On Tue, Jun 13, 2023 at 2:16 PM Segher Boessenkool wrote: > > Hi! > > On Tue, Jun 13, 2023 at 10:15:49AM +0800, Jiufu Guo wrote: > > David Edelsohn writes: > > > > > > This definitely seems to be a better solution. > > > > > > The TARGET_CONST_ANCHOR change should not be part of this patch. Also > > > there is no ChangeLog for the patch. > > > > Thanks a lot for your quick review!! And sorry for the sending this patch > > in a hurry. I would update the patch accordingly. > > > > This generally looks correct and consistent with other ports. I want > > > to give Segher a chance to double check it, if he wishes. > > The documentation is very clear that the only thing for which you can > have BLKmode is "mem". Not unspec, only "mem". > > Let's not do this. The existing code has clear and obvious semantics, > which is documented as well -- there is no reason to make it worse in > every respect. Segher, Unfortunately, GCC now is inconsistent and this response is incorrect. The documentation is out of date or was ignored and the "facts on the ground" contradict your review. Yes, (const_int 0) is supposed to be a general no-op and BLKmode only is supposed to be used for MEM, but other major targets (arm, aarch64, riscv, s390) all use unspec:BLK and specifically UNSPEC_TIE. rs6000 is the only port that does not follow this convention. The middle-end has adapted to the behavior of all of the other targets, whether that conformed to the documentation or not. The rs6000 port needs to be fixed and Jiufu's approach is the correct one, consistent with all other targets for stack tie. If the documentation differs, the documentation needs to be updated, not a different approach for the rs6000 port. Jiufu's patch is correct. Thanks, David
Re: [PATCH 1/4] rs6000: build constant via li;rotldi
On Mon, Jun 12, 2023 at 11:30 PM Jiufu Guo wrote:
>
>
> Hi David,
>
> David Edelsohn writes:
> > On Wed, Jun 7, 2023 at 9:55 PM Jiufu Guo wrote:
> >
> > Hi,
> >
> > This patch checks if a constant is possible to be rotated to/from a
> > positive
> > or negative value from "li". If so, we could use "li;rotldi" to build it.
> >
> > Bootstrap and regtest pass on ppc64{,le}.
> > Is this ok for trunk?
> >
> > BR,
> > Jeff (Jiufu)
> >
> > gcc/ChangeLog:
> >
> > * config/rs6000/rs6000.cc (can_be_rotated_to_positive_li): New
> > function.
> > (can_be_rotated_to_negative_li): New function.
> > (can_be_built_by_li_and_rotldi): New function.
> > (rs6000_emit_set_long_const): Call can_be_built_by_li_and_rotldi.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/powerpc/const-build.c: New test.
> > ---
> > gcc/config/rs6000/rs6000.cc | 64 +--
> > .../gcc.target/powerpc/const-build.c | 54
> > 2 files changed, 112 insertions(+), 6 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.target/powerpc/const-build.c
> >
> > diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> > index 42f49e4a56b..1dd0072350a 100644
> > --- a/gcc/config/rs6000/rs6000.cc
> > +++ b/gcc/config/rs6000/rs6000.cc
> > @@ -10258,6 +10258,48 @@ rs6000_emit_set_const (rtx dest, rtx source)
> > return true;
> > }
> >
> > +/* Check if C can be rotated to a positive value which 'li' instruction
> > + is able to load. If so, set *ROT to the number by which C is rotated,
> > + and return true. Return false otherwise. */
> > +
> > +static bool
> > +can_be_rotated_to_positive_li (HOST_WIDE_INT c, int *rot)
> > +{
> > + /* 49 leading zeros and 15 low bits on the positive value
> > + generated by 'li' instruction. */
> > + return can_be_rotated_to_lowbits (c, 15, rot);
> > +}
> > +
> > +/* Like can_be_rotated_to_positive_li, but check the negative value of
> > 'li'. */
> > +
> > +static bool
> > +can_be_rotated_to_negative_li (HOST_WIDE_INT c, int *rot)
> > +{
> > + return can_be_rotated_to_lowbits (~c, 15, rot);
> > +}
> > +
> > +/* Check if value C can be built by 2 instructions: one is 'li', another
> > is
> > + rotldi.
> > +
> > + If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
> > + is set to -1, and return true. Return false otherwise. */
> > +
> >
> > I look at this feature and it's good, but I don't fully understand the
> > benefit of this level of abstraction. Ideally all of the above functions
> > would
> > be inlined. They aren't reused.
> >
> > +static bool
> > +can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
> > + HOST_WIDE_INT *mask)
> > +{
> > + int n;
> > + if (can_be_rotated_to_positive_li (c, )
> > + || can_be_rotated_to_negative_li (c, ))
> >
> > Why not
> >
> > /* Check if C or ~C can be rotated to a positive or negative value
> > which 'li' instruction is able to load. */
> > if (can_be_rotated_to_lowbits (c, 15, )
> > || can_be_rotated_to_lowbits (~c, 15, ))
>
>
> Thanks a lot for your review!!
>
> Your suggestions could also achieve my goal of using a new function:
> Using "can_be_rotated_to_positive_li" is just trying to get a
> straightforward name. Like yours, the code's comments would also
> make it easy to understand.
I recognize that you are trying to be consistent with the other
functions that you add in later patches, but it feels like overkill in
abstraction to me. Or maybe combine postive_li and negative_li into a
single function so that the abstraction serves a purpose other than a
tail call and creating an alias for a specific invocation of
can_be_rotated_to_lowbits.
Thanks, David
>
> BR,
> Jeff (Jiufu Guo)
> >
> > ...
> >
> > This is a style of software engineering, but it seems overkill to me when
> > the function is a single line that tail calls another function. Am I
> > missing
> > something?
> >
> > The rest of this patch looks good.
> >
> > Thanks, David
> >
> > +{
> > + *mask = HOST_WIDE_INT_M1;
> > + *shift = HOST_BITS_PER_WIDE_INT - n;
> > + return true;
> > +}
> > +
> >
Re: [PATCH] rs6000: replace '(const_int 0)' to 'unspec:BLK [(const_int 0)]' for stack_tie
Hi, Jiufu
This definitely seems to be a better solution.
The TARGET_CONST_ANCHOR change should not be part of this patch. Also
there is no ChangeLog for the patch.
This generally looks correct and consistent with other ports. I want
to give Segher a chance to double check it, if he wishes.
Thanks David
On Mon, Jun 12, 2023 at 9:19 AM Jiufu Guo wrote:
>
> Hi,
>
> For stack_tie, currently below insn is generated:
> (insn 15 14 16 3 (parallel [
> (set (mem/c:BLK (reg/f:DI 1 1) [1 A8])
> (const_int 0 [0]))
> ]) "/home/guojiufu/temp/gdb.c":13:3 922 {stack_tie}
> (nil))
>
> It is "set (mem/c:BLK (reg/f:DI 1 1) (const_int 0 [0])". This maybe
> looks like "a memory block is zerored", while actually stack_tie
> may be more like a placeholder, and does not generate any thing.
>
> To avoid potential misunderstand, "UNPSEC:BLK [(const_int 0)].." could
> be used here like other ports.
>
> This patch does this. Bootstrap pass on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu Guo)
>
> ---
> gcc/config/rs6000/predicates.md | 11 +++
> gcc/config/rs6000/rs6000-logue.cc | 4 +++-
> gcc/config/rs6000/rs6000.cc | 4
> gcc/config/rs6000/rs6000.md | 14 ++
> 4 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/config/rs6000/predicates.md b/gcc/config/rs6000/predicates.md
> index a16ee30f0c0..4748cb37ce8 100644
> --- a/gcc/config/rs6000/predicates.md
> +++ b/gcc/config/rs6000/predicates.md
> @@ -1854,10 +1854,13 @@ (define_predicate "stmw_operation"
> (define_predicate "tie_operand"
>(match_code "parallel")
> {
> - return (GET_CODE (XVECEXP (op, 0, 0)) == SET
> - && MEM_P (XEXP (XVECEXP (op, 0, 0), 0))
> - && GET_MODE (XEXP (XVECEXP (op, 0, 0), 0)) == BLKmode
> - && XEXP (XVECEXP (op, 0, 0), 1) == const0_rtx);
> + rtx set = XVECEXP (op, 0, 0);
> + return (GET_CODE (set) == SET
> + && MEM_P (SET_DEST (set))
> + && GET_MODE (SET_DEST (set)) == BLKmode
> + && GET_CODE (SET_SRC (set)) == UNSPEC
> + && XINT (SET_SRC (set), 1) == UNSPEC_TIE
> + && XVECEXP (SET_SRC (set), 0, 0) == const0_rtx);
> })
>
> ;; Match a small code model toc reference (or medium and large
> diff --git a/gcc/config/rs6000/rs6000-logue.cc
> b/gcc/config/rs6000/rs6000-logue.cc
> index bc6b153b59f..b99f43a8282 100644
> --- a/gcc/config/rs6000/rs6000-logue.cc
> +++ b/gcc/config/rs6000/rs6000-logue.cc
> @@ -1463,7 +1463,9 @@ rs6000_emit_stack_tie (rtx fp, bool hard_frame_needed)
>while (--i >= 0)
> {
>rtx mem = gen_frame_mem (BLKmode, regs[i]);
> - RTVEC_ELT (p, i) = gen_rtx_SET (mem, const0_rtx);
> + RTVEC_ELT (p, i)
> + = gen_rtx_SET (mem, gen_rtx_UNSPEC (BLKmode, gen_rtvec (1,
> const0_rtx),
> + UNSPEC_TIE));
> }
>
>emit_insn (gen_stack_tie (gen_rtx_PARALLEL (VOIDmode, p)));
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index d197c3f3289..0c81ebea711 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -1760,6 +1760,10 @@ static const struct attribute_spec
> rs6000_attribute_table[] =
>
> #undef TARGET_UPDATE_IPA_FN_TARGET_INFO
> #define TARGET_UPDATE_IPA_FN_TARGET_INFO rs6000_update_ipa_fn_target_info
> +
> +#undef TARGET_CONST_ANCHOR
> +#define TARGET_CONST_ANCHOR 0x8000
> +
>
>
> /* Processor table. */
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index b0db8ae508d..fdcf8347812 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -158,6 +158,7 @@ (define_c_enum "unspec"
> UNSPEC_HASHCHK
> UNSPEC_XXSPLTIDP_CONST
> UNSPEC_XXSPLTIW_CONST
> + UNSPEC_TIE
>])
>
> ;;
> @@ -10828,7 +10829,9 @@ (define_expand "restore_stack_block"
>operands[4] = gen_frame_mem (Pmode, operands[1]);
>p = rtvec_alloc (1);
>RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
> - const0_rtx);
> + gen_rtx_UNSPEC (BLKmode,
> + gen_rtvec (1, const0_rtx),
> + UNSPEC_TIE));
>operands[5] = gen_rtx_PARALLEL (VOIDmode, p);
> })
>
> @@ -10866,7 +10869,9 @@ (define_expand "restore_stack_nonlocal"
>operands[5] = gen_frame_mem (Pmode, operands[3]);
>p = rtvec_alloc (1);
>RTVEC_ELT (p, 0) = gen_rtx_SET (gen_frame_mem (BLKmode, operands[0]),
> - const0_rtx);
> + gen_rtx_UNSPEC (BLKmode,
> + gen_rtvec (1, const0_rtx),
> + UNSPEC_TIE));
>operands[6] = gen_rtx_PARALLEL (VOIDmode, p);
> })
>
> @@ -13898,7 +13903,8 @@ (define_insn "*save_fpregs__r1"
> ; not be moved over loads from or stores to stack memory.
>
[PATCH, AIX] Debugging does not require a stack frame.
The rs6000 port has allocated a stack frame when debugging is enabled on AIX since the earliest versions of the port. Apparently the earliest versions of the debuggers for AIX had difficulty with stackless frames. Both AIX DBX and GDB support stackless frames on AIX, and IBM XLC, OpenXL and LLVM for AIX do not generate an extraneous stack frame when debugging is enabled. This patch updates the rs6000 stack info function to not set the.stack frame flag when debugging is enabled for AIX. Bootstrapped on powerpc-ibm-aix7.2.5.0 Committed. Thanks, David * gcc/config/rs6000/rs6000-logue.cc (rs6000_stack_info): Do not require a stack frame when debugging is enabled for AIX. index bc6b153b59f..98846f781ec 100644 --- a/gcc/config/rs6000/rs6000-logue.cc +++ b/gcc/config/rs6000/rs6000-logue.cc @@ -928,9 +928,6 @@ rs6000_stack_info (void) else if (frame_pointer_needed) info->push_p = 1; - else if (TARGET_XCOFF && write_symbols != NO_DEBUG && !flag_compare_debug) -info->push_p = 1; - else info->push_p = non_fixed_size > (TARGET_32BIT ? 220 : 288);
Re: [PATCH] rs6000: Guard __builtin_{un, }pack_vector_int128 with vsx [PR109932]
On Tue, Jun 6, 2023 at 5:19 AM Kewen.Lin wrote:
> Hi,
>
> As PR109932 shows, builtins __builtin_{un,}pack_vector_int128
> should be guarded under vsx rather than power7, as their
> corresponding bif patterns have the conditions TARGET_VSX
> and VECTOR_MEM_ALTIVEC_OR_VSX_P (V1TImode). This patch is to
> move __builtin_{un,}pack_vector_int128 to stanza vsx to ensure
> their supports.
>
> Bootstrapped and regtested on powerpc64-linux-gnu P7/P8/P9 and
> powerpc64le-linux-gnu P9 and P10.
>
> I'll push this next week if no objections.
>
> BR,
> Kewen
> -
> PR target/109932
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000-builtins.def (__builtin_pack_vector_int128,
> __builtin_unpack_vector_int128): Move from stanza power7 to vsx.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/powerpc/pr109932-1.c: New test.
> * gcc.target/powerpc/pr109932-2.c: New test.
>
This is okay.
Thanks, David
> ---
> gcc/config/rs6000/rs6000-builtins.def | 14 +++---
> gcc/testsuite/gcc.target/powerpc/pr109932-1.c | 16
> gcc/testsuite/gcc.target/powerpc/pr109932-2.c | 16
> 3 files changed, 39 insertions(+), 7 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr109932-1.c
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr109932-2.c
>
> diff --git a/gcc/config/rs6000/rs6000-builtins.def
> b/gcc/config/rs6000/rs6000-builtins.def
> index 92d9b46e1b9..a38184b0ef9 100644
> --- a/gcc/config/rs6000/rs6000-builtins.def
> +++ b/gcc/config/rs6000/rs6000-builtins.def
> @@ -2009,6 +2009,13 @@
>const vsll __builtin_vsx_xxspltd_2di (vsll, const int<1>);
> XXSPLTD_V2DI vsx_xxspltd_v2di {}
>
> + const vsq __builtin_pack_vector_int128 (unsigned long long, \
> + unsigned long long);
> +PACK_V1TI packv1ti {}
> +
> + const unsigned long __builtin_unpack_vector_int128 (vsq, const int<1>);
> +UNPACK_V1TI unpackv1ti {}
> +
>
> ; Power7 builtins (ISA 2.06).
> [power7]
> @@ -2030,16 +2037,9 @@
>const unsigned int __builtin_divweu (unsigned int, unsigned int);
> DIVWEU diveu_si {}
>
> - const vsq __builtin_pack_vector_int128 (unsigned long long, \
> - unsigned long long);
> -PACK_V1TI packv1ti {}
> -
>void __builtin_ppc_speculation_barrier ();
> SPECBARR speculation_barrier {}
>
> - const unsigned long __builtin_unpack_vector_int128 (vsq, const int<1>);
> -UNPACK_V1TI unpackv1ti {}
> -
>
> ; Power7 builtins requiring 64-bit GPRs (even with 32-bit addressing).
> [power7-64]
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr109932-1.c
> b/gcc/testsuite/gcc.target/powerpc/pr109932-1.c
> new file mode 100644
> index 000..3e3f9eaa65e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr109932-1.c
> @@ -0,0 +1,16 @@
> +/* { dg-require-effective-target powerpc_altivec_ok } */
> +/* { dg-options "-maltivec -mno-vsx" } */
> +
> +/* Verify there is no ICE but one expected error message instead. */
> +
> +#include
> +
> +extern vector signed __int128 res_vslll;
> +extern unsigned long long aull[2];
> +
> +void
> +testVectorInt128Pack ()
> +{
> + res_vslll = __builtin_pack_vector_int128 (aull[0], aull[1]); /* {
> dg-error "'__builtin_pack_vector_int128' requires the '-mvsx' option" } */
> +}
> +
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr109932-2.c
> b/gcc/testsuite/gcc.target/powerpc/pr109932-2.c
> new file mode 100644
> index 000..3e3f9eaa65e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr109932-2.c
> @@ -0,0 +1,16 @@
> +/* { dg-require-effective-target powerpc_altivec_ok } */
> +/* { dg-options "-maltivec -mno-vsx" } */
> +
> +/* Verify there is no ICE but one expected error message instead. */
> +
> +#include
> +
> +extern vector signed __int128 res_vslll;
> +extern unsigned long long aull[2];
> +
> +void
> +testVectorInt128Pack ()
> +{
> + res_vslll = __builtin_pack_vector_int128 (aull[0], aull[1]); /* {
> dg-error "'__builtin_pack_vector_int128' requires the '-mvsx' option" } */
> +}
> +
> --
> 2.25.1
>
Re: [PATCH] rs6000: Don't use TFmode for 128 bits fp constant in toc [PR110011]
On Tue, Jun 6, 2023 at 5:20 AM Kewen.Lin wrote:
> Hi,
>
> As PR110011 shows, when encoding 128 bits fp constant into
> toc, we adopts REAL_VALUE_TO_TARGET_LONG_DOUBLE which is
> to find the first float mode with LONG_DOUBLE_TYPE_SIZE
> bits of precision, it would be TFmode here. But the 128
> bits fp constant can be with mode IFmode or KFmode, which
> doesn't necessarily have the same underlying float format
> as the one of TFmode, like this PR exposes, with option
> -mabi=ibmlongdouble TFmode has ibm_extended_format while
> KFmode has ieee_quad_format, mixing up the formats (the
> encoding/decoding ways) would cause unexpected results.
>
> This patch is to make it use constant's own mode instead
> of TFmode for real_to_target call.
>
> Bootstrapped and regtested on powerpc64-linux-gnu P7/P8/P9 and
> powerpc64le-linux-gnu P9 and P10.
>
> I'll push this next week if no objections.
>
> BR,
> Kewen
> -
> PR target/110011
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.cc (output_toc): Use its own mode of the
> 128-bit float constant for real_to_target call.
>
The comment wording can be worded better. Maybe
Use the mode of the 128-bit floating constant itself for real_to_target
call.
This is okay.
Thanks, David
> gcc/testsuite/ChangeLog:
>
> * gcc.target/powerpc/pr110011.c: New test.
> ---
> gcc/config/rs6000/rs6000.cc | 2 +-
> gcc/testsuite/gcc.target/powerpc/pr110011.c | 42 +
> 2 files changed, 43 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr110011.c
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 3f129ea37d2..330c6a6fa5f 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -17314,7 +17314,7 @@ output_toc (FILE *file, rtx x, int labelno,
> machine_mode mode)
>if (DECIMAL_FLOAT_MODE_P (GET_MODE (x)))
> REAL_VALUE_TO_TARGET_DECIMAL128 (*CONST_DOUBLE_REAL_VALUE (x), k);
>else
> - REAL_VALUE_TO_TARGET_LONG_DOUBLE (*CONST_DOUBLE_REAL_VALUE (x), k);
> + real_to_target (k, CONST_DOUBLE_REAL_VALUE (x), GET_MODE (x));
>
>if (TARGET_64BIT)
> {
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr110011.c
> b/gcc/testsuite/gcc.target/powerpc/pr110011.c
> new file mode 100644
> index 000..5b04d3e298a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr110011.c
> @@ -0,0 +1,42 @@
> +/* { dg-do run } */
> +/* { dg-require-effective-target float128_runtime } */
> +/* Force long double to be with IBM format here, to verify
> + _Float128 constant still uses its own format (IEEE) for
> + encoding rather than IBM format. */
> +/* { dg-options "-mfp-in-toc -mabi=ibmlongdouble" } */
> +/* { dg-add-options float128 } */
> +
> +#define MPFR_FLOAT128_MAX 0x1.p+16383f128
> +
> +__attribute__ ((noipa))
> +_Float128 f128_max ()
> +{
> + return MPFR_FLOAT128_MAX;
> +}
> +
> +typedef union
> +{
> + int w[4];
> + _Float128 f128;
> +} U;
> +
> +int main ()
> +{
> +
> + U umax;
> + umax.f128 = f128_max ();
> + /* ieee float128 max:
> + 7ffe . */
> + if (umax.w[1] != 0x || umax.w[2] != 0x)
> +__builtin_abort ();
> +#ifdef __LITTLE_ENDIAN__
> + if (umax.w[0] != 0x || umax.w[3] != 0x7ffe)
> +__builtin_abort ();
> +#else
> + if (umax.w[3] != 0x || umax.w[0] != 0x7ffe)
> +__builtin_abort ();
> +#endif
> +
> + return 0;
> +}
> +
> --
> 2.31.1
>
Re: [PATCH 4/4] rs6000: build constant via li/lis;rldic
On Wed, Jun 7, 2023 at 9:56 PM Jiufu Guo wrote:
> Hi,
>
> This patch checks if a constant is possible to be built by "li;rldic".
> We only need to take care of "negative li", other forms do not need to
> check.
> For example, "negative lis" is just a "negative li" with an additional
> shift.
>
> Bootstrap and regtest pass on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu)
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.cc (can_be_built_by_li_and_rldic): New
> function.
> (rs6000_emit_set_long_const): Call can_be_built_by_li_and_rldic.
>
This is okay.
Do you have any measurement of how expensive it is to test all of these
additional methods to generate a constant? How much does this affect the
compile time?
Thanks, David
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/powerpc/const-build.c: Add more tests.
> ---
> gcc/config/rs6000/rs6000.cc | 61 ++-
> .../gcc.target/powerpc/const-build.c | 28 +
> 2 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 2a3fa733b45..cd04b6b5c82 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10387,6 +10387,64 @@ can_be_built_by_li_lis_and_rldicr (HOST_WIDE_INT
> c, int *shift,
>return false;
> }
>
> +/* Check if value C can be built by 2 instructions: one is 'li', another
> is
> + rldic.
> +
> + If so, *SHIFT is set to the 'shift' operand of rldic; and *MASK is set
> + to the mask value about the 'mb' operand of rldic; and return true.
> + Return false otherwise. */
> +
> +static bool
> +can_be_built_by_li_and_rldic (HOST_WIDE_INT c, int *shift, HOST_WIDE_INT
> *mask)
> +{
> + /* There are 49 successive ones in the negative value of 'li'. */
> + int ones = 49;
> +
> + /* 1..1xx1..1: negative value of li --> 0..01..1xx0..0:
> + right bits are shifted as 0's, and left 1's(and x's) are cleaned. */
> + int tz = ctz_hwi (c);
> + int lz = clz_hwi (c);
> + int middle_ones = clz_hwi (~(c << lz));
> + if (tz + lz + middle_ones >= ones)
> +{
> + *mask = ((1LL << (HOST_BITS_PER_WIDE_INT - tz - lz)) - 1LL) << tz;
> + *shift = tz;
> + return true;
> +}
> +
> + /* 1..1xx1..1 --> 1..1xx0..01..1: some 1's(following x's) are cleaned.
> */
> + int leading_ones = clz_hwi (~c);
> + int tailing_ones = ctz_hwi (~c);
> + int middle_zeros = ctz_hwi (c >> tailing_ones);
> + if (leading_ones + tailing_ones + middle_zeros >= ones)
> +{
> + *mask = ~(((1ULL << middle_zeros) - 1ULL) << tailing_ones);
> + *shift = tailing_ones + middle_zeros;
> + return true;
> +}
> +
> + /* xx1..1xx: --> xx0..01..1xx: some 1's(following x's) are cleaned. */
> + /* Get the position for the first bit of successive 1.
> + The 24th bit would be in successive 0 or 1. */
> + HOST_WIDE_INT low_mask = (1LL << 24) - 1LL;
> + int pos_first_1 = ((c & (low_mask + 1)) == 0)
> + ? clz_hwi (c & low_mask)
> + : HOST_BITS_PER_WIDE_INT - ctz_hwi (~(c | low_mask));
> + middle_ones = clz_hwi (~c << pos_first_1);
> + middle_zeros = ctz_hwi (c >> (HOST_BITS_PER_WIDE_INT - pos_first_1));
> + if (pos_first_1 < HOST_BITS_PER_WIDE_INT
> + && middle_ones + middle_zeros < HOST_BITS_PER_WIDE_INT
> + && middle_ones + middle_zeros >= ones)
> +{
> + *mask = ~(((1ULL << middle_zeros) - 1LL)
> + << (HOST_BITS_PER_WIDE_INT - pos_first_1));
> + *shift = HOST_BITS_PER_WIDE_INT - pos_first_1 + middle_zeros;
> + return true;
> +}
> +
> + return false;
> +}
> +
> /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
> Output insns to set DEST equal to the constant C as a series of
> lis, ori and shl instructions. */
> @@ -10435,7 +10493,8 @@ rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)
> }
>else if (can_be_built_by_li_lis_and_rotldi (c, , )
>|| can_be_built_by_li_lis_and_rldicl (c, , )
> - || can_be_built_by_li_lis_and_rldicr (c, , ))
> + || can_be_built_by_li_lis_and_rldicr (c, , )
> + || can_be_built_by_li_and_rldic (c, , ))
> {
>temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
>unsigned HOST_WIDE_INT imm = (c | ~mask);
> diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c
> b/gcc/testsuite/gcc.target/powerpc/const-build.c
> index 8c209921d41..b503ee31c7c 100644
> --- a/gcc/testsuite/gcc.target/powerpc/const-build.c
> +++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
> @@ -82,6 +82,29 @@ lis_rldicr_12 (void)
>return 0x5310LL;
> }
>
> +long long NOIPA
> +li_rldic_13 (void)
> +{
> + return 0x000f8531LL;
> +}
> +long long NOIPA
> +li_rldic_14 (void)
> +{
> + return 0x853100ffLL;
> +}
> +
> +long long NOIPA
> +li_rldic_15 (void)
> +{
> + return 0x8031LL;
> +}
> +
> +long long NOIPA
> +li_rldic_16 (void)
> +{
>
Re: [PATCH 3/4] rs6000: build constant via li/lis;rldicl/rldicr
On Wed, Jun 7, 2023 at 9:56 PM Jiufu Guo wrote:
> Hi,
>
> This patch checks if a constant is possible left/right cleaned on a rotated
> value from a negative value of "li/lis". If so, we can build the constant
> through "li/lis ; rldicl/rldicr".
>
> Bootstrap and regtest pass on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu)
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.cc (can_be_built_by_li_lis_and_rldicl): New
> function.
> (can_be_built_by_li_lis_and_rldicr): New function.
> (rs6000_emit_set_long_const): Call
> can_be_built_by_li_lis_and_rldicr and
> can_be_built_by_li_lis_and_rldicl.
>
This is okay. See below.
Thanks, David
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/powerpc/const-build.c: Add more tests.
> ---
> gcc/config/rs6000/rs6000.cc | 61 ++-
> .../gcc.target/powerpc/const-build.c | 44 +
> 2 files changed, 104 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 03cd9d5e952..2a3fa733b45 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10332,6 +10332,61 @@ can_be_built_by_li_lis_and_rotldi (HOST_WIDE_INT
> c, int *shift,
>return false;
> }
>
> +/* Check if value C can be built by 2 instructions: one is 'li or lis',
> + another is rldicl.
> +
> + If so, *SHIFT is set to the shift operand of rldicl, and *MASK is set
> to
> + the mask operand of rldicl, and return true.
> + Return false otherwise. */
> +
> +static bool
> +can_be_built_by_li_lis_and_rldicl (HOST_WIDE_INT c, int *shift,
> + HOST_WIDE_INT *mask)
> +{
> + /* Leading zeros may be cleaned by rldicl with a mask. Change leading
> zeros
> + to ones and then recheck it. */
> + int lz = clz_hwi (c);
> + HOST_WIDE_INT unmask_c
> += c | (HOST_WIDE_INT_M1U << (HOST_BITS_PER_WIDE_INT - lz));
> + int n;
> + if (can_be_rotated_to_negative_li (unmask_c, )
>
using can_be_rotated_to_lowbits (~unmask_c, 15, )
Maybe Segher would want the abstraction, but it seems more wasteful to me.
> + || can_be_rotated_to_negative_lis (unmask_c, ))
> +{
> + *mask = HOST_WIDE_INT_M1U >> lz;
> + *shift = n == 0 ? 0 : HOST_BITS_PER_WIDE_INT - n;
> + return true;
> +}
> +
> + return false;
> +}
> +
> +/* Check if value C can be built by 2 instructions: one is 'li or lis',
> + another is rldicr.
> +
> + If so, *SHIFT is set to the shift operand of rldicr, and *MASK is set
> to
> + the mask operand of rldicr, and return true.
> + Return false otherwise. */
> +
> +static bool
> +can_be_built_by_li_lis_and_rldicr (HOST_WIDE_INT c, int *shift,
> + HOST_WIDE_INT *mask)
> +{
> + /* Tailing zeros may be cleaned by rldicr with a mask. Change tailing
> zeros
> + to ones and then recheck it. */
> + int tz = ctz_hwi (c);
> + HOST_WIDE_INT unmask_c = c | ((HOST_WIDE_INT_1U << tz) - 1);
> + int n;
> + if (can_be_rotated_to_negative_li (unmask_c, )
> + || can_be_rotated_to_negative_lis (unmask_c, ))
> +{
> + *mask = HOST_WIDE_INT_M1U << tz;
> + *shift = HOST_BITS_PER_WIDE_INT - n;
> + return true;
> +}
> +
> + return false;
> +}
> +
> /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
> Output insns to set DEST equal to the constant C as a series of
> lis, ori and shl instructions. */
> @@ -10378,7 +10433,9 @@ rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)
>emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
> GEN_INT ((ud2 ^ 0x) << 16)));
> }
> - else if (can_be_built_by_li_lis_and_rotldi (c, , ))
> + else if (can_be_built_by_li_lis_and_rotldi (c, , )
> + || can_be_built_by_li_lis_and_rldicl (c, , )
> + || can_be_built_by_li_lis_and_rldicr (c, , ))
> {
>temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
>unsigned HOST_WIDE_INT imm = (c | ~mask);
> @@ -10387,6 +10444,8 @@ rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)
>emit_move_insn (temp, GEN_INT (imm));
>if (shift != 0)
> temp = gen_rtx_ROTATE (DImode, temp, GEN_INT (shift));
> + if (mask != HOST_WIDE_INT_M1)
> + temp = gen_rtx_AND (DImode, temp, GEN_INT (mask));
>emit_move_insn (dest, temp);
> }
>else if (ud3 == 0 && ud4 == 0)
> diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c
> b/gcc/testsuite/gcc.target/powerpc/const-build.c
> index c38a1dd91f2..8c209921d41 100644
> --- a/gcc/testsuite/gcc.target/powerpc/const-build.c
> +++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
> @@ -46,6 +46,42 @@ lis_rotldi_6 (void)
>return 0x5318LL;
> }
>
> +long long NOIPA
> +li_rldicl_7 (void)
> +{
> + return 0x3ffa1LL;
> +}
> +
> +long long NOIPA
> +li_rldicl_8 (void)
> +{
> + return 0xff8531LL;
> +}
> +
>
Re: [PATCH 2/4] rs6000: build constant via lis;rotldi
On Wed, Jun 7, 2023 at 9:55 PM Jiufu Guo wrote:
> Hi,
>
> This patch checks if a constant is possible to be rotated to/from a
> negative
> value from "lis". If so, we could use "lis;rotldi" to build it.
> The positive value of "lis" does not need to be analyzed. Because if a
> constant can be rotated from the positive value of "lis", it also can be
> rotated from a positive value of "li".
>
> Bootstrap and regtest pass on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu)
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.cc (can_be_rotated_to_negative_lis): New
> function.
> (can_be_built_by_li_and_rotldi): Rename to ...
> (can_be_built_by_li_lis_and_rotldi): ... this function.
> (rs6000_emit_set_long_const): Call
> can_be_built_by_li_lis_and_rotldi.
>
This patch is okay.
Thanks, David
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/powerpc/const-build.c: Add more tests.
> ---
> gcc/config/rs6000/rs6000.cc | 42 ---
> .../gcc.target/powerpc/const-build.c | 16 ++-
> 2 files changed, 52 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 1dd0072350a..03cd9d5e952 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10278,19 +10278,51 @@ can_be_rotated_to_negative_li (HOST_WIDE_INT c,
> int *rot)
>return can_be_rotated_to_lowbits (~c, 15, rot);
> }
>
> -/* Check if value C can be built by 2 instructions: one is 'li', another
> is
> - rotldi.
> +/* Check if C can be rotated to a negative value which 'lis' instruction
> is
> + able to load: 1..1xx0..0. If so, set *ROT to the number by which C is
> + rotated, and return true. Return false otherwise. */
> +
> +static bool
> +can_be_rotated_to_negative_lis (HOST_WIDE_INT c, int *rot)
> +{
> + /* case a. 1..1xxx0..01..1: up to 15 x's, at least 16 0's. */
> + int leading_ones = clz_hwi (~c);
> + int tailing_ones = ctz_hwi (~c);
> + int middle_zeros = ctz_hwi (c >> tailing_ones);
> + if (middle_zeros >= 16 && leading_ones + tailing_ones >= 33)
> +{
> + *rot = HOST_BITS_PER_WIDE_INT - tailing_ones;
> + return true;
> +}
> +
> + /* case b. xx0..01..1xx: some of 15 x's (and some of 16 0's) are
> + rotated over the highest bit. */
> + int pos_one = clz_hwi ((c << 16) >> 16);
> + middle_zeros = ctz_hwi (c >> (HOST_BITS_PER_WIDE_INT - pos_one));
> + int middle_ones = clz_hwi (~(c << pos_one));
> + if (middle_zeros >= 16 && middle_ones >= 33)
> +{
> + *rot = pos_one;
> + return true;
> +}
> +
> + return false;
> +}
> +
> +/* Check if value C can be built by 2 instructions: one is 'li or lis',
> + another is rotldi.
>
> If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
> is set to -1, and return true. Return false otherwise. */
>
> static bool
> -can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
> +can_be_built_by_li_lis_and_rotldi (HOST_WIDE_INT c, int *shift,
>HOST_WIDE_INT *mask)
> {
>int n;
>if (can_be_rotated_to_positive_li (c, )
> - || can_be_rotated_to_negative_li (c, ))
> + || can_be_rotated_to_negative_li (c, )
> + || can_be_rotated_to_negative_lis (c, ))
> {
>*mask = HOST_WIDE_INT_M1;
>*shift = HOST_BITS_PER_WIDE_INT - n;
> @@ -10346,7 +10378,7 @@ rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)
>emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
> GEN_INT ((ud2 ^ 0x) << 16)));
> }
> - else if (can_be_built_by_li_and_rotldi (c, , ))
> + else if (can_be_built_by_li_lis_and_rotldi (c, , ))
> {
>temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
>unsigned HOST_WIDE_INT imm = (c | ~mask);
> diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c
> b/gcc/testsuite/gcc.target/powerpc/const-build.c
> index 70f095f6bf2..c38a1dd91f2 100644
> --- a/gcc/testsuite/gcc.target/powerpc/const-build.c
> +++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
> @@ -34,14 +34,28 @@ li_rotldi_4 (void)
>return 0x2194LL;
> }
>
> +long long NOIPA
> +lis_rotldi_5 (void)
> +{
> + return 0x8531LL;
> +}
> +
> +long long NOIPA
> +lis_rotldi_6 (void)
> +{
> + return 0x5318LL;
> +}
> +
> struct fun arr[] = {
>{li_rotldi_1, 0x75310LL},
>{li_rotldi_2, 0x2164LL},
>{li_rotldi_3, 0x8531LL},
>{li_rotldi_4, 0x2194LL},
> + {lis_rotldi_5, 0x8531LL},
> + {lis_rotldi_6, 0x5318LL},
> };
>
> -/* { dg-final { scan-assembler-times {\mrotldi\M} 4 } } */
> +/* { dg-final { scan-assembler-times {\mrotldi\M} 6 } } */
>
> int
> main ()
> --
> 2.39.1
>
>
Re: [PATCH 1/4] rs6000: build constant via li;rotldi
On Wed, Jun 7, 2023 at 9:55 PM Jiufu Guo wrote:
> Hi,
>
> This patch checks if a constant is possible to be rotated to/from a
> positive
> or negative value from "li". If so, we could use "li;rotldi" to build it.
>
> Bootstrap and regtest pass on ppc64{,le}.
> Is this ok for trunk?
>
> BR,
> Jeff (Jiufu)
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000.cc (can_be_rotated_to_positive_li): New
> function.
> (can_be_rotated_to_negative_li): New function.
> (can_be_built_by_li_and_rotldi): New function.
> (rs6000_emit_set_long_const): Call can_be_built_by_li_and_rotldi.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/powerpc/const-build.c: New test.
> ---
> gcc/config/rs6000/rs6000.cc | 64 +--
> .../gcc.target/powerpc/const-build.c | 54
> 2 files changed, 112 insertions(+), 6 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/const-build.c
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 42f49e4a56b..1dd0072350a 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10258,6 +10258,48 @@ rs6000_emit_set_const (rtx dest, rtx source)
>return true;
> }
>
> +/* Check if C can be rotated to a positive value which 'li' instruction
> + is able to load. If so, set *ROT to the number by which C is rotated,
> + and return true. Return false otherwise. */
> +
> +static bool
> +can_be_rotated_to_positive_li (HOST_WIDE_INT c, int *rot)
> +{
> + /* 49 leading zeros and 15 low bits on the positive value
> + generated by 'li' instruction. */
> + return can_be_rotated_to_lowbits (c, 15, rot);
> +}
> +
> +/* Like can_be_rotated_to_positive_li, but check the negative value of
> 'li'. */
> +
> +static bool
> +can_be_rotated_to_negative_li (HOST_WIDE_INT c, int *rot)
> +{
> + return can_be_rotated_to_lowbits (~c, 15, rot);
> +}
> +
> +/* Check if value C can be built by 2 instructions: one is 'li', another
> is
> + rotldi.
> +
> + If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
> + is set to -1, and return true. Return false otherwise. */
> +
>
I look at this feature and it's good, but I don't fully understand the
benefit of this level of abstraction. Ideally all of the above functions
would be inlined. They aren't reused.
> +static bool
> +can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
> + HOST_WIDE_INT *mask)
> +{
> + int n;
> + if (can_be_rotated_to_positive_li (c, )
> + || can_be_rotated_to_negative_li (c, ))
>
Why not
/* Check if C or ~C can be rotated to a positive or negative value
which 'li' instruction is able to load. */
if (can_be_rotated_to_lowbits (c, 15, )
|| can_be_rotated_to_lowbits (~c, 15, ))
...
This is a style of software engineering, but it seems overkill to me when
the function is a single line that tail calls another function. Am I
missing something?
The rest of this patch looks good.
Thanks, David
> +{
> + *mask = HOST_WIDE_INT_M1;
> + *shift = HOST_BITS_PER_WIDE_INT - n;
> + return true;
> +}
> +
> + return false;
> +}
> +
> /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
> Output insns to set DEST equal to the constant C as a series of
> lis, ori and shl instructions. */
> @@ -10266,15 +10308,14 @@ static void
> rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
> {
>rtx temp;
> + int shift;
> + HOST_WIDE_INT mask;
>HOST_WIDE_INT ud1, ud2, ud3, ud4;
>
>ud1 = c & 0x;
> - c = c >> 16;
> - ud2 = c & 0x;
> - c = c >> 16;
> - ud3 = c & 0x;
> - c = c >> 16;
> - ud4 = c & 0x;
> + ud2 = (c >> 16) & 0x;
> + ud3 = (c >> 32) & 0x;
> + ud4 = (c >> 48) & 0x;
>
>if ((ud4 == 0x && ud3 == 0x && ud2 == 0x && (ud1 & 0x8000))
>|| (ud4 == 0 && ud3 == 0 && ud2 == 0 && ! (ud1 & 0x8000)))
> @@ -10305,6 +10346,17 @@ rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)
>emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
> GEN_INT ((ud2 ^ 0x) << 16)));
> }
> + else if (can_be_built_by_li_and_rotldi (c, , ))
> +{
> + temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
> + unsigned HOST_WIDE_INT imm = (c | ~mask);
> + imm = (imm >> shift) | (imm << (HOST_BITS_PER_WIDE_INT - shift));
> +
> + emit_move_insn (temp, GEN_INT (imm));
> + if (shift != 0)
> + temp = gen_rtx_ROTATE (DImode, temp, GEN_INT (shift));
> + emit_move_insn (dest, temp);
> +}
>else if (ud3 == 0 && ud4 == 0)
> {
>temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
> diff --git a/gcc/testsuite/gcc.target/powerpc/const-build.c
> b/gcc/testsuite/gcc.target/powerpc/const-build.c
> new file mode 100644
> index 000..70f095f6bf2
> --- /dev/null
> +++
Re: [PATCH 1/4] rs6000: build constant via li;rotldi
Hi, Jiufu
* config/rs6000/rs6000.cc (can_be_rotated_to_possitive_li): New
function.
(can_be_rotated_to_negative_li): New function.
(can_be_built_by_li_and_rotldi): New function.
(rs6000_emit_set_long_const): Call can_be_built_by_li_and_rotldi.
In English the word "positive" contains one "s", not two. Please
correct throughout the patches.
Also a style issue, comments before a function should be followed by a
blank line.
> +/* Check if C can be rotated to a possitive value which 'li' instruction
positive
> + is able to load. If so, set *ROT to the number by which C is rotated,
> + and return true. Return false otherwise. */
Add a blank line here
> +static bool
> +can_be_rotated_to_possitive_li (HOST_WIDE_INT c, int *rot)
positive
> +{
> + /* 49 leading zeros and 15 lowbits on the possitive value
low bits, positive
> + generated by 'li' instruction. */
> + return can_be_rotated_to_lowbits (c, 15, rot);
> +}
> +/* Check if value C can be built by 2 instructions: one is 'li', another is
> + rotldi.
> +
> + If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
> + is set to -1, and return true. Return false otherwise. */
> +static bool
> +can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
> +HOST_WIDE_INT *mask)
> +{
> + int n;
> + if (can_be_rotated_to_possitive_li (c, )
> + || can_be_rotated_to_negative_li (c, ))
> +{
> + *mask = HOST_WIDE_INT_M1;
> + *shift = HOST_BITS_PER_WIDE_INT - n;
> + return true;
> +}
> +
> + return false;
> +}
> +
> /* Subroutine of rs6000_emit_set_const, handling PowerPC64 DImode.
> Output insns to set DEST equal to the constant C as a series of
> lis, ori and shl instructions. */
> @@ -10246,15 +10285,14 @@ static void> rs6000_emit_set_long_const (rtx dest,
> HOST_WIDE_INT c)> {>rtx temp;> + int shift;> + HOST_WIDE_INT mask;>
> HOST_WIDE_INT ud1, ud2, ud3, ud4;> >ud1 = c & 0x;> - c = c >> 16;>
> - ud2 = c & 0x;> - c = c >> 16;> - ud3 = c & 0x;> - c = c >> 16;>
> - ud4 = c & 0x;> + ud2 = (c >> 16) & 0x;> + ud3 = (c >> 32) &
> 0x;> + ud4 = (c >> 48) & 0x;> >if ((ud4 == 0x && ud3 ==
> 0x && ud2 == 0x && (ud1 & 0x8000))>|| (ud4 == 0 && ud3 == 0
> && ud2 == 0 && ! (ud1 & 0x8000)))> @@ -10278,6 +10316,19 @@
> rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)>emit_move_insn
> (dest, gen_rtx_XOR (DImode, temp,>
> GEN_INT ((ud2 ^ 0x) << 16)));> }> + else if
> (can_be_built_by_li_and_rotldi (c, , ))> +{> + temp =
> !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);> + unsigned
> HOST_WIDE_INT imm = (c | ~mask);> + imm = (imm >> shift) | (imm <<
> (HOST_BITS_PER_WIDE_INT - shift));> +> + emit_move_insn (temp, GEN_INT
> (imm));> + if (shift != 0)> + temp = gen_rtx_ROTATE (DImode, temp,
> GEN_INT (shift));> + if (mask != HOST_WIDE_INT_M1)
How is mask != HOST_WIDE_INT_M1? The call to
can_by_built_by_li_and_rotldi() set it
to that value and it is not modified in the interim statements.
> + temp = gen_rtx_AND (DImode, temp, GEN_INT (mask));> +
> emit_move_insn (dest, temp);> +}>else if (ud3 == 0 && ud4 == 0)>
> {>temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
Thanks, David
Re: ping^^: [PATCH] rs6000: Enable const_anchor for 'addi'
On Tue, May 30, 2023 at 11:00 PM Jiufu Guo wrote:
>
> Gentle ping...
>
> Jiufu Guo via Gcc-patches writes:
>
> > Gentle ping...
> >
> > Jiufu Guo via Gcc-patches writes:
> >
> >> Hi,
> >>
> >> I'm thinking that we may enable this patch for stage1, so ping it.
> >> https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603530.html
> >>
> >> BR,
> >> Jeff (Jiufu)
> >>
> >> Jiufu Guo writes:
> >>
> >>> Hi,
> >>>
> >>> There is a functionality as const_anchor in cse.cc. This const_anchor
> >>> supports to generate new constants through adding small gap/offsets to
> >>> existing constant. For example:
> >>>
> >>> void __attribute__ ((noinline)) foo (long long *a)
> >>> {
> >>> *a++ = 0x2351847027482577LL;
> >>> *a++ = 0x2351847027482578LL;
> >>> }
> >>> The second constant (0x2351847027482578LL) can be compated by adding
> '1'
> >>> to the first constant (0x2351847027482577LL).
> >>> This is profitable if more than one instructions are need to build the
> >>> second constant.
> >>>
> >>> * For rs6000, we can enable this functionality, as the instruction
> >>> 'addi' is just for this when gap is smaller than 0x8000.
> >>>
> >>> * Besides enabling TARGET_CONST_ANCHOR on rs6000, this patch also fixed
> >>> one issue. The issue is:
> >>> "gcc_assert (SCALAR_INT_MODE_P (mode))" is an requirement for function
> >>> "try_const_anchors".
> >>>
> >>> * One potential side effect of this patch:
> >>> Comparing with
> >>> "r101=0x2351847027482577LL
> >>> ...
> >>> r201=0x2351847027482578LL"
> >>> The new r201 will be "r201=r101+1", and then r101 will live longer,
> >>> and would increase pressure when allocating registers.
> >>> But I feel, this would be acceptable for this const_anchor feature.
> >>>
> >>> * With this patch, I checked the performance change on SPEC2017, while,
> >>> and the performance is not aggressive, since this functionality is not
> >>> hit on any hot path. There are runtime wavings/noise(e.g. on
> >>> povray_r/xalancbmk_r/xz_r), that are not caused by the patch.
> >>>
> >>> With this patch, I also checked the changes in object files (from
> >>> GCC bootstrap and SPEC), the significant changes are the improvement
> >>> that: "addi" vs. "2 or more insns: lis+or.."; it also exposes some
> >>> other optimizations opportunities: like combine/jump2. While the
> >>> code to store/load one more register is also occurring in few cases,
> >>> but it does not impact overall performance.
> >>>
> >>> * To refine this patch, some history discussions are referenced:
> >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=33699
> >>> https://gcc.gnu.org/pipermail/gcc-patches/2009-April/260421.html
> >>> https://gcc.gnu.org/pipermail/gcc-patches/2021-March/566744.html
> >>>
> >>>
> >>> Bootstrap and regtest pass on ppc64 and ppc64le for this patch.
> >>> Is this ok for trunk?
>
Hi, Jiufu
Thanks for developing this patch and your persistence.
The rs6000.cc part of the patch (TARGET_CONST_ANCHOR) is okay for Stage 1.
This is approved.
I don't have the authority to approve the change to cse_insn. Is the
cse_insn change a prerequisite? Will the rs6000 change break or produce
wrong code without the cse change? The second part of the patch should be
posted separately to the mailing list, with a cc for appropriate
maintainers, because most maintainers will not be following this specific
thread to approve the other part of the patch.
Thanks, David
> >>>
> >>>
> >>> BR,
> >>> Jeff (Jiufu)
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> * config/rs6000/rs6000.cc (TARGET_CONST_ANCHOR): New define.
> >>> * cse.cc (cse_insn): Add guard condition.
> >>>
> >>> gcc/testsuite/ChangeLog:
> >>>
> >>> * gcc.target/powerpc/const_anchors.c: New test.
> >>> * gcc.target/powerpc/try_const_anchors_ice.c: New test.
> >>>
> >>> ---
> >>> gcc/config/rs6000/rs6000.cc | 4
> >>> gcc/cse.cc| 3 ++-
> >>> .../gcc.target/powerpc/const_anchors.c| 20 +++
> >>> .../powerpc/try_const_anchors_ice.c | 16 +++
> >>> 4 files changed, 42 insertions(+), 1 deletion(-)
> >>> create mode 100644 gcc/testsuite/gcc.target/powerpc/const_anchors.c
> >>> create mode 100644
> gcc/testsuite/gcc.target/powerpc/try_const_anchors_ice.c
> >>>
> >>> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> >>> index d2743f7bce6..80cded6dec1 100644
> >>> --- a/gcc/config/rs6000/rs6000.cc
> >>> +++ b/gcc/config/rs6000/rs6000.cc
> >>> @@ -1760,6 +1760,10 @@ static const struct attribute_spec
> rs6000_attribute_table[] =
> >>>
> >>> #undef TARGET_UPDATE_IPA_FN_TARGET_INFO
> >>> #define TARGET_UPDATE_IPA_FN_TARGET_INFO
> rs6000_update_ipa_fn_target_info
> >>> +
> >>> +#undef TARGET_CONST_ANCHOR
> >>> +#define TARGET_CONST_ANCHOR 0x8000
> >>> +
> >>>
> >>>
> >>> /* Processor table. */
> >>> diff --git a/gcc/cse.cc b/gcc/cse.cc
> >>> index b13afd4ba72..56542b91c1e 100644
> >>> --- a/gcc/cse.cc
> >>> +++ b/gcc/cse.cc
> >>> @@
Re: [PATCH 7/7] Expand directly for single bit test
On Sun, May 21, 2023 at 11:25 AM Andrew Pinski wrote: > On Sun, May 21, 2023 at 11:17 AM David Edelsohn via Gcc-patches > wrote: > > > > Hi, Andrew > > > > Thanks for this series of patches to improve do_store_flag. > Unfortunately > > this specific patch in the series has caused a bootstrap failure on > > powerpc-aix. I bisected this failure to this specific patch. Note that I > > am building as 32 bit, so this could be a specific issue about bit size. > > > > * expr.cc (fold_single_bit_test): Rename to ... > > (expand_single_bit_test): This and expand directly. > > (do_store_flag): Update for the rename function. > > Did this include the fix I did for big-endian at > r14-1022-g7f3df8e65c71e5 ? I had found that I broke big-endian last > night with that patch and pushed the fix once I figured out what I did > wrong. > If you already tried post the fix, I will try to look into it as soon > as possible. > > The big-endian patch fixed the issue for Power also. Thanks, David
Re: [PATCH 7/7] Expand directly for single bit test
Hi, Andrew Thanks for this series of patches to improve do_store_flag. Unfortunately this specific patch in the series has caused a bootstrap failure on powerpc-aix. I bisected this failure to this specific patch. Note that I am building as 32 bit, so this could be a specific issue about bit size. * expr.cc (fold_single_bit_test): Rename to ... (expand_single_bit_test): This and expand directly. (do_store_flag): Update for the rename function. Thanks, David
Re: [PATCH 5/5] match.pd: Use splits in makefile and make configurable.
On Fri, May 5, 2023 at 11:38 AM Tamar Christina
wrote:
> > -Original Message-
> > From: Jakub Jelinek
> > Sent: Friday, May 5, 2023 4:33 PM
> > To: Tamar Christina
> > Cc: Jeff Law ; David Edelsohn >;
> > GCC Patches
> > Subject: Re: [PATCH 5/5] match.pd: Use splits in makefile and make
> > configurable.
> >
> > On Fri, May 05, 2023 at 03:22:11PM +, Tamar Christina wrote:
> > > > We require GNU make, so perhaps we could use something like
> > > > $(wordlist
> > > > 1,$(NUM_MATCH_SPLITS),$(check_p_numbers))
> > > > instead of
> > > > $(shell seq 1 $(NUM_MATCH_SPLITS))
> > > > provided we move the check_p_numbers definition earlier (or perhaps
> > > > bettter rename it to something more generic, so that it is clear
> > > > that is a variable holding numbers from 1 to .
> > >
> > > I'm currently testing
> > >
> > > NUM_MATCH_SPLITS = @DEFAULT_MATCHPD_PARTITIONS@ -
> > MATCH_SPLITS_SEQ =
> > > $(shell seq 1 $(NUM_MATCH_SPLITS))
> > > +MATCH_SPLITS_SEQ = $(shell echo {1..$(NUM_MATCH_SPLITS)})
> > >
> > > Which seems to work since it looks like we require an sh compatible
> shell.
> > >
> > > Question is this right? From the existing
> >
> > AIX /bin/sh certainly doesn't handle that.
>
> Wow, wonder what sh version it has..
>
> >
> > But what do I know about AIX...
>
> Same..
>
AIX defaults to Korn Shell.
I always use Bash on AIX to build GCC and recommend Bash in the GCC build
instructions for AIX.
Do we want to require Bash? Bash is a more self-contained requirement than
seq from coreutils.
Thanks, David
>
> >
> > This seems to work and we use it already in the Makefile.
> > If something else works portably, we could change both spots...
> >
> > 2023-05-05 Jakub Jelinek
> >
> > * Makefile.in (check_p_numbers): Rename to one_to_, move
> > earlier with helper variables also renamed.
> > (MATCH_SPLUT_SEQ): Use $(wordlist
> > 1,$(NUM_MATCH_SPLITS),$(one_to_))
> > instead of $(shell seq 1 $(NUM_MATCH_SPLITS)).
> > (check_p_subdirs): Use $(one_to_) instead of
> > $(check_p_numbers).
> >
> > --- gcc/Makefile.in.jj2023-05-05 16:02:37.180575333 +0200
> > +++ gcc/Makefile.in 2023-05-05 17:20:27.923251821 +0200
> > @@ -214,9 +214,19 @@ rtl-ssa-warn = $(STRICT_WARN)
> > GCC_WARN_CFLAGS = $(LOOSE_WARN) $(C_LOOSE_WARN) $($(@D)-warn)
> > $(if $(filter-out $(STRICT_WARN),$($(@D)-warn)),,$(C_STRICT_WARN))
> > $(NOCOMMON_FLAG) $($@-warn) GCC_WARN_CXXFLAGS =
> > $(LOOSE_WARN) $($(@D)-warn) $(NOCOMMON_FLAG) $($@-warn)
> >
> > +# 1 2 3 ...
> > +one_to__0:=1 2 3 4 5 6 7 8 9
> > +one_to__1:=0 $(one_to__0)
> > +one_to__2:=$(foreach i,$(one_to__0),$(addprefix
> > +$(i),$(one_to__1))) one_to__3:=$(addprefix
> > 0,$(one_to__1))
> > +$(one_to__2) one_to__4:=$(foreach
> > +i,$(one_to__0),$(addprefix $(i),$(one_to__3)))
> > +one_to__5:=$(addprefix 0,$(one_to__3)) $(one_to__4)
> > +one_to__6:=$(foreach i,$(one_to__0),$(addprefix
> > +$(i),$(one_to__5)))
> > +one_to_:=$(one_to__0) $(one_to__2) $(one_to__4)
> > +$(one_to__6)
> > +
> > # The number of splits to be made for the match.pd files.
> > NUM_MATCH_SPLITS = @DEFAULT_MATCHPD_PARTITIONS@ -
> > MATCH_SPLITS_SEQ = $(shell seq 1 $(NUM_MATCH_SPLITS))
> > +MATCH_SPLITS_SEQ = $(wordlist
> > 1,$(NUM_MATCH_SPLITS),$(one_to_))
> > GIMPLE_MATCH_PD_SEQ_SRC = $(patsubst %, gimple-match-%.cc,
> > $(MATCH_SPLITS_SEQ)) GIMPLE_MATCH_PD_SEQ_O = $(patsubst %, gimple-
> > match-%.o, $(MATCH_SPLITS_SEQ)) GENERIC_MATCH_PD_SEQ_SRC =
> > $(patsubst %, generic-match-%.cc, $(MATCH_SPLITS_SEQ)) @@ -4234,18
> > +4244,10 @@ $(patsubst %,%-subtargets,$(lang_checks)
> > check_p_tool=$(firstword $(subst _, ,$*))
> > check_p_count=$(check_$(check_p_tool)_parallelize)
> > check_p_subno=$(word 2,$(subst _, ,$*))
> > -check_p_numbers0:=1 2 3 4 5 6 7 8 9
> > -check_p_numbers1:=0 $(check_p_numbers0) -
> > check_p_numbers2:=$(foreach i,$(check_p_numbers0),$(addprefix
> > $(i),$(check_p_numbers1))) -check_p_numbers3:=$(addprefix
> > 0,$(check_p_numbers1)) $(check_p_numbers2) -
> > check_p_numbers4:=$(foreach i,$(check_p_numbers0),$(addprefix
> > $(i),$(check_p_numbers3))) -check_p_numbers5:=$(addprefix
> > 0,$(check_p_numbers3)) $(check_p_numbers4) -
> > check_p_numbers6:=$(foreach i,$(check_p_num
Re: [PATCH 5/5] match.pd: Use splits in makefile and make configurable.
This patch has broken GCC bootstrap on AIX. It appears to rely upon, or complain about, the command "seq": /nasfarm/edelsohn/install/GCC12/bin/g++ -std=c++11 -g -DIN_GCC -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wno-format -Wmissing-format-attribute -Wconditionally-supported -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common -DHAVE_CONFIG_H -DGENERATOR_FILE -static-libstdc++ -static-libgcc -Wl,-bbigtoc -Wl,-bmaxdata:0x4000 -o build/genmatch \ build/genmatch.o ../build-powerpc-ibm-aix7.2.5.0/libcpp/libcpp.a build/errors.o build/vec.o build/hash-table.o build/sort.o ../build-powerpc-ibm-aix7.2.5.0/libiberty/libiberty.a /usr/bin/bash: seq: command not found /usr/bin/bash: seq: command not found build/genmatch --gimple \ --header=tmp-gimple-match-auto.h --include=gimple-match-auto.h \ /nasfarm/edelsohn/src/src/gcc/match.pd All of the match files are dumped to stdout. Thanks, David
Re: [PATCH, wwwdocs] readings: Update AIX linker links
On Mon, Jan 30, 2023 at 4:52 PM Gerald Pfeifer wrote: > Hi David, > > the noticed the links to the AIX 4.3 and AIX 5L docs were broken and could > not find a good replacement, though I did find one for AIX 7.2. > > How about the patch below? > Hi, Gerald That change is fine with me. Thanks, David > > Or should we omit such links? (Or do you have recommendations?) > > Thanks, > Gerald > > > diff --git a/htdocs/readings.html b/htdocs/readings.html > index 3f41ef2a..0a978e8f 100644 > --- a/htdocs/readings.html > +++ b/htdocs/readings.html > @@ -270,8 +270,7 @@ names. >Manufacturer: IBM, Motorola >https://www.ibm.com/systems/power/openpower/;>Power > ISA >https://openpowerfoundation.org/?resource_lib=64-bit-elf-v2-abi-specification-power-architecture;>64-Bit > ELF V2 ABI - OpenPOWER ABI > - http://publib16.boulder.ibm.com/pseries/en_US/infocenter/base/43_docs/aixassem/alangref/toc.htm;>AIX > V4.3 Assembler Language Ref. > - http://publibn.boulder.ibm.com/doc_link/en_US/a_doc_lib/aixassem/alangref/alangreftfrm.htm;>AIX > 5L Assembler Language Ref. > + https://www.ibm.com/docs/en/ssw_aix_72/assembler/assembler_pdf.pdf;>AIX > 7.2 Assembler Language Reference > > > rx >
Re: [PATCH RFC] c++: implement P1492 contracts
This patch broke bootstrap on AIX due to its use of strchrnul(). It is an extension available in Linux, but not all targets. /src/src/gcc/cp/contracts.cc:213:21: error: 'strchrnul' was not declared in this scope; did you mean 'strchr'? 213 | size_t role_len = strchrnul (role, ':') - role; | ^ | strchr Would you please avoid the extension or provide an implementation? Thanks David
Re: [PATCHv2, rs6000] Enable have_cbranchcc4 on rs6000
On Fri, Nov 18, 2022 at 7:20 AM Segher Boessenkool <
seg...@kernel.crashing.org> wrote:
> On Fri, Nov 18, 2022 at 02:35:30PM +0800, HAO CHEN GUI wrote:
> > 在 2022/11/17 21:24, David Edelsohn 写道:
> > > Why are you using zero_constant predicate instead of matching
> (const_int 0) for operand 2?
> > The "const_int 0" is an operand other than a predicate. We need a
> predicate here.
>
> Said differently, it is passed as an operand to this named pattern or
> optab, so you need a match_operand here.
>
Earlier versions of patterns for other targets used (const_int 0), but they
seem to have changed that, so match_operand is needed.
Thanks, David
>
> > > Why does this need the new all_branch_comparison_operator? Can the
> ifcvt optimization correctly elide the 2 insn sequence?
> > Because rs6000 defines "*cbranch_2insn" insn, such insns are generated
> after expand.
> >
> > (jump_insn 50 47 51 11 (set (pc)
> > (if_then_else (ge (reg:CCFP 156)
> > (const_int 0 [0]))
> > (label_ref 53)
> > (pc)))
> "/home/guihaoc/gcc/gcc-mainline-base/gmp/mpz/cmpabs_d.c":80:7 884
> {*cbranch_2insn}
> > (expr_list:REG_DEAD (reg:CCFP 156)
> > (int_list:REG_BR_PROB 633507684 (nil)))
> > -> 53)
>
> But notice the cost of *cbranch_2insn -- ifcvt should never generate
> cbranchcc4 with such composite conditions!
>
> > In prepare_cmp_insn, the comparison is verified by insn_operand_matches.
> If
> > extra_insn_branch_comparison_operator is not included in "cbranchcc4"
> predicate,
> > it hits ICE here.
> >
> > if (GET_MODE_CLASS (mode) == MODE_CC)
> > {
> > enum insn_code icode = optab_handler (cbranch_optab, CCmode);
> > test = gen_rtx_fmt_ee (comparison, VOIDmode, x, y);
> > gcc_assert (icode != CODE_FOR_nothing
> > && insn_operand_matches (icode, 0, test));
> > *ptest = test;
> > return;
> > }
> >
> > The real conditional move is generated by emit_conditional_move_1.
> Commonly
> > "*cbranch_2insn" can't be optimized out and it returns NULL_RTX.
> >
> > if (COMPARISON_P (comparison))
> > {
> > saved_pending_stack_adjust save;
> > save_pending_stack_adjust ();
> > last = get_last_insn ();
> > do_pending_stack_adjust ();
> > machine_mode cmpmode = comp.mode;
> > prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1),
> > GET_CODE (comparison), NULL_RTX, unsignedp,
> > OPTAB_WIDEN, , );
> > if (comparison)
> > {
> >rtx res = emit_conditional_move_1 (target, comparison,
> > op2, op3, mode);
> >if (res != NULL_RTX)
> > return res;
> > }
> > delete_insns_since (last);
> > restore_pending_stack_adjust ();
> >
> > I think that extra_insn_branch_comparison_operator should be included in
> > "cbranchcc4" predicates as such insns exist. And leave it to
> > emit_conditional_move which decides whether it can be optimized or not.
>
> I don't think we should pretend we have any conditional jumps the
> machine does not actually have, in cbranchcc4. When would this ever be
> useful? cror;beq can be quite expensive, compared to the code it would
> replace anyway.
>
> If something generates those here (which then ICEs later), that is
> wrong, fix *that*? Is it ifcvt doing it?
>
>
> Segher
>
Re: [PATCHv2, rs6000] Enable have_cbranchcc4 on rs6000
On Thu, Nov 17, 2022 at 1:39 AM HAO CHEN GUI wrote:
> Hi,
> The patch enables have_cbrnachcc4 which is a flag in ifcvt.cc to
> indicate if branch by CC bits is invalid or not. The new expand pattern
> "cbranchcc4" is created which intend to match the pattern defined in
> "*cbranch", "*cbranch_2insn" and "*creturn". The operand sequence in
> "cbranchcc4" is inline with the definition in gccint. And the operand
> sequence doesn't matter in pattern matching. So I think it should work.
>
> Compared to last version, one new predicate and one new expander are
> created.
>
> Bootstrapped and tested on powerpc64-linux BE and LE with no regressions.
> Is this okay for trunk? Any recommendations? Thanks a lot.
>
> ChangeLog
> 2022-11-17 Haochen Gui
>
> gcc/
> * config/rs6000/predicates.md (all_branch_comparison_operator):
> New,
> and includes operators in branch_comparison_operator and
> extra_insn_branch_comparison_operator.
> * config/rs6000/rs6000.md (cbranchcc4): New expand pattern.
>
> gcc/testsuite/
> * gcc.target/powerpc/cbranchcc4.c: New.
>
>
> patch.diff
> diff --git a/gcc/config/rs6000/predicates.md
> b/gcc/config/rs6000/predicates.md
> index b1fcc69bb60..843b6f39b84 100644
> --- a/gcc/config/rs6000/predicates.md
> +++ b/gcc/config/rs6000/predicates.md
> @@ -1308,6 +1308,7 @@ (define_special_predicate "equality_operator"
>
> ;; Return 1 if OP is a comparison operation that is valid for a branch
> ;; instruction. We check the opcode against the mode of the CC value.
> +
> ;; validate_condition_mode is an assertion.
> (define_predicate "branch_comparison_operator"
> (and (match_operand 0 "comparison_operator")
> @@ -1331,6 +1332,11 @@ (define_predicate
> "extra_insn_branch_comparison_operator"
> GET_MODE (XEXP (op, 0))),
> 1")))
>
> +;; Return 1 if OP is a comparison operation that is valid for a branch.
> +(define_predicate "all_branch_comparison_operator"
> + (ior (match_operand 0 "branch_comparison_operator")
> + (match_operand 0 "extra_insn_branch_comparison_operator")))
> +
> ;; Return 1 if OP is an unsigned comparison operator.
> (define_predicate "unsigned_comparison_operator"
>(match_code "ltu,gtu,leu,geu"))
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index e9e5cd1e54d..7b7d747a85d 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -13067,6 +13067,16 @@ (define_insn_and_split "*_cc"
> ;; Conditional branches.
> ;; These either are a single bc insn, or a bc around a b.
>
> +(define_expand "cbranchcc4"
> + [(set (pc)
> + (if_then_else (match_operator 0 "all_branch_comparison_operator"
> + [(match_operand 1 "cc_reg_operand")
> +(match_operand 2 "zero_constant")])
> + (label_ref (match_operand 3))
> + (pc)))]
> + ""
> + "")
> +
>
This is better, but the pattern should be near and after the existing
cbranch4 patterns earlier in the file, not the *cbranch pattern. It
doesn't match the comment.
Why are you using zero_constant predicate instead of matching (const_int 0)
for operand 2?
Why does this need the new all_branch_comparison_operator? Can the ifcvt
optimization correctly elide the 2 insn sequence?
Thanks, David
> (define_insn "*cbranch"
>[(set (pc)
> (if_then_else (match_operator 1 "branch_comparison_operator"
> diff --git a/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c
> b/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c
> new file mode 100644
> index 000..528ba1a878d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-rtl-ce1" } */
> +/* { dg-final { scan-rtl-dump "noce_try_store_flag_constants" "ce1" } } */
> +
> +/* The inner branch should be detected by ifcvt then be converted to a
> setcc
> + with a plus by noce_try_store_flag_constants. */
> +
> +int test (unsigned int a, unsigned int b)
> +{
> +return (a < b ? 0 : (a > b ? 2 : 1));
> +}
>
Re: [rs6000, patch] Enable have_cbranchcc4 on rs6000
Hao cbranchcc4 is a named pattern and requires a specific operand ordering. If you change *cbranch to cbranchcc4, you must change the order of the operands, not a quick and dirty hack to *cbranch. Also, you should change *cbranch_2insn and *creturn as well so that all of the patterns are consistent. See for example the aarch64.md implementation. and the documentation in Standard Names https://gcc.gnu.org/onlinedocs/gccint/Standard-Names.html which mentions cbranch4 and, briefly, cbranchcc4. You seemed to want to make the minimal change so that the pattern would work with ifcvt without considering the impact on the existing pattern and without understanding what a named pattern with specific operands really means. You changed the pattern predicate so that the operands in the wrong positions would match the pattern. Thanks, David On Wed, Nov 16, 2022 at 12:56 AM HAO CHEN GUI wrote: > Hi David, > I found definition of the operands in 'cbranch'. The argumnets matters. > I will create a new expand pattern for cbranchcc4. Thanks a lot for your > comments. > > 'cbranchmode4’ > Conditional branch instruction combined with a compare instruction. > Operand 0 is a comparison operator. Operand 1 and operand 2 are the > first and second operands of the comparison, respectively. Operand 3 > is the code_label to jump to. > > Gui Haochen > Thanks > > 在 2022/11/16 11:04, David Edelsohn 写道: > > It's great to add cbranchcc4 to the Power port where it definitely was > an omission, but adapting *cbranch for that purpose is the wrong approach. > The changes to the pattern are incorrect because they are covering up a > difference in ordering of the operands. One can argue that the named > pattern only enables the functionality in ifcvt and the pattern otherwise > is used in its previous role. But this is a Frankenstein monster > approach. You're trying to twist the existing pattern so that it triggers > as cbranchcc4, but creating a pattern that messes up its arguments and only > works because the new, named pattern never is called. This is too ugly. > Please fix. >
Re: [rs6000, patch] Enable have_cbranchcc4 on rs6000
On Tue, Nov 15, 2022 at 9:32 PM HAO CHEN GUI wrote:
> Hi,
> The patch enables have_cbrnachcc4 which is a flag in ifcvt.cc to
> indicate if branch by CC bits is invalid or not. As rs6000 already has
> "*cbranch" insn which does branching according to CC bits, the flag
> should be enabled and relevant branches can be optimized out. The test
> case illustrates the optimization.
>
> "*cbranch" is an anonymous insn which can't be generated directly.
> So changing "const_int 0" to the third operand predicated by
> "zero_constant" won't cause ICEs as orginal patterns still can be matched.
>
> Bootstrapped and tested on powerpc64-linux BE and LE with no regressions.
> Is this okay for trunk? Any recommendations? Thanks a lot.
>
>
> ChangeLog
> 2022-11-16 Haochen Gui
>
> gcc/
> * config/rs6000/rs6000.md (*cbranch): Rename to...
> (cbranchcc4): ...this, and set const_int 0 to the third operand.
>
> gcc/testsuite/
> * gcc.target/powerpc/cbranchcc4.c: New.
>
>
> patch.diff
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index e9e5cd1e54d..ee171f21f6a 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -13067,11 +13067,11 @@ (define_insn_and_split "*_cc"
> ;; Conditional branches.
> ;; These either are a single bc insn, or a bc around a b.
>
> -(define_insn "*cbranch"
> +(define_insn "cbranchcc4"
>[(set (pc)
> (if_then_else (match_operator 1 "branch_comparison_operator"
> [(match_operand 2 "cc_reg_operand"
> "y")
> - (const_int 0)])
> + (match_operand 3 "zero_constant")])
> (label_ref (match_operand 0))
> (pc)))]
>""
>
Shouldn't cbranchcc4 be a separate pattern? This pattern has an existing
purpose and an expected ordering of operands.
cbranchcc4 is passed the comparison operator as operand 0. Other ports
ignore the second comparison operator and use (const_int 0). Operand 3 is
the label, which seems to be the reason that you needed to change it to
match_operand 3.
It's great to add cbranchcc4 to the Power port where it definitely was an
omission, but adapting *cbranch for that purpose is the wrong approach.
The changes to the pattern are incorrect because they are covering up a
difference in ordering of the operands. One can argue that the named
pattern only enables the functionality in ifcvt and the pattern otherwise
is used in its previous role. But this is a Frankenstein monster
approach. You're trying to twist the existing pattern so that it triggers
as cbranchcc4, but creating a pattern that messes up its arguments and only
works because the new, named pattern never is called. This is too ugly.
Please fix.
Thanks, David
diff --git a/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c
> b/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c
> new file mode 100644
> index 000..1751d274bbf
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/cbranchcc4.c
> @@ -0,0 +1,8 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-rtl-ce1" } */
> +/* { dg-final {scan-rtl-dump "noce_try_store_flag_constants" "ce1" } } */
> +
> +int test (unsigned int a, unsigned int b)
> +{
> +return (a < b ? 0 : (a > b ? 2 : 1));
> +}
>
Re: [PATCH V2] Enable small loop unrolling for O2
> This patch does not change rs6000/s390 since I don't have machines to > test them, but I suppose the default behavior is the same since they > enable flag_unroll_loops at O2. There are Power (rs6000) systems in the Compile Farm. Trial Linux on Z (s390x) VMs are available through the Linux Community Cloud. https://linuxone.cloud.marist.edu/#/register?flag=VM Thanks, David
Re: [PATCH] libstdc++: Allow emergency EH alloc pool size to be tuned [PR68606]
This patch seems to have broken bootstrap on AIX. It seems to assume
methods that aren't guaranteed to be defined.
Thanks, David
libtool: compile: /tmp/GCC/./gcc/xgcc -B/tmp/GCC/./gcc/
-B/nasfarm/edelsohn/ins
tall/GCC/powerpc-ibm-aix7.2.5.0/bin/
-B/nasfarm/edelsohn/install/GCC/powerpc-ibm
-aix7.2.5.0/lib/ -isystem
/nasfarm/edelsohn/install/GCC/powerpc-ibm-aix7.2.5.0/i
nclude -isystem
/nasfarm/edelsohn/install/GCC/powerpc-ibm-aix7.2.5.0/sys-include
-fno-checking -DHAVE_CONFIG_H -I..
-I/nasfarm/edelsohn/src/src/libstdc++-v3/../
libiberty -I/nasfarm/edelsohn/src/src/libstdc++-v3/../include
-D_GLIBCXX_SHARED
-I/tmp/GCC/powerpc-ibm-aix7.2.5.0/libstdc++-v3/include/powerpc-ibm-aix7.2.5.0
-I
/tmp/GCC/powerpc-ibm-aix7.2.5.0/libstdc++-v3/include
-I/nasfarm/edelsohn/src/src
/libstdc++-v3/libsupc++ -I/nasfarm/edelsohn/install/include
-I/nasfarm/edelsohn/
install/include -g -O2 -DIN_GLIBCPP_V3 -Wno-error -c cp-demangle.c -fPIC
-DPIC -o cp-demangle.o
/nasfarm/edelsohn/src/src/libstdc++-v3/libsupc++/eh_alloc.cc: In member
function 'void* {anonymous}::pool::allocate(std::size_t)':
/nasfarm/edelsohn/src/src/libstdc++-v3/libsupc++/eh_alloc.cc:239:54: error:
no matching function for call to
'__gnu_cxx::__scoped_lock::__scoped_lock(int&)'
239 | __gnu_cxx::__scoped_lock sentry(emergency_mutex);
| ^
In file included from
/nasfarm/edelsohn/src/src/libstdc++-v3/libsupc++/eh_alloc.cc:37:
/tmp/GCC/powerpc-ibm-aix7.2.5.0/libstdc++-v3/include/ext/concurrence.h:240:14:
note: candidate: '__gnu_cxx::__scoped_lock::__scoped_lock(__mutex_type&)'
240 | explicit __scoped_lock(__mutex_type& __name) : _M_device(__name)
| ^
/tmp/GCC/powerpc-ibm-aix7.2.5.0/libstdc++-v3/include/ext/concurrence.h:240:42:
note: no known conversion for argument 1 from 'int' to
'__gnu_cxx::__scoped_lock::__mutex_type&'
240 | explicit __scoped_lock(__mutex_type& __name) : _M_device(__name)
|~~^~
/tmp/GCC/powerpc-ibm-aix7.2.5.0/libstdc++-v3/include/ext/concurrence.h:236:5:
note: candidate: '__gnu_cxx::__scoped_lock::__scoped_lock(const
__gnu_cxx::__scoped_lock&)'
236 | __scoped_lock(const __scoped_lock&);
| ^
/tmp/GCC/powerpc-ibm-aix7.2.5.0/libstdc++-v3/include/ext/concurrence.h:236:19:
note: no known conversion for argument 1 from 'int' to 'const
__gnu_cxx::__scoped_lock&'
236 | __scoped_lock(const __scoped_lock&);
| ^~~~
make[5]: *** [Makefile:778: eh_alloc.lo] Error 1
Re: [PATCH] Set discriminators for call stmts on the same line within the same basic block
This patch causes a bootstrap comparison failure on AIX. It apparently does not cause a failure on PPC64BE Linux with the same ABI, so I suspect that the failure may be related to the way that function aliases are implemented on AIX, which doesn't have ELF symbol alias semantics. "This change will also simplify call site lookups since now location with discriminator will uniquely identify the call site (no callee function name is needed)." I will open a PR with more information about the comparison difference now that I have a work-around to bring AIX back to a bootstrappable state. Any thoughts about what could be going wrong? Thanks, David
Re: [PATCH] Avoid depending on destructor order
On Fri, Sep 23, 2022 at 10:12 AM Thomas Neumann wrote: > > > > +static const bool in_shutdown = false; > > > > I'll let Jason or others decide if this is the right solution. It seems > > that in_shutdown also could be declared outside the #ifdef and > > initialized as "false". > > sure, either is fine. Moving it outside the #ifdef wastes one byte in > the executable (while the compiler can eliminate the const), but it does > not really matter. > > I have verified that the patch below fixes builds for both fast-path and > non-fast-path builds. But if you prefer I will move the in_shutdown > definition instead. > > Best > > Thomas > > PS: in_shutdown is an int here instead of a bool because non-fast-path > builds do not include stdbool. Not a good reason, of course, but I > wanted to keep the patch minimal and it makes no difference in practice. > > > When using the atomic fast path deregistering can fail during > program shutdown if the lookup structures are already destroyed. > The assert in __deregister_frame_info_bases takes that into > account. In the non-fast-path case however is not aware of > program shutdown, which caused a compiler error on such platforms. > We fix that by introducing a constant for in_shutdown in > non-fast-path builds. > > libgcc/ChangeLog: > * unwind-dw2-fde.c: Introduce a constant for in_shutdown > for the non-fast-path case. > > diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c > index d237179f4ea..0bcd5061d76 100644 > --- a/libgcc/unwind-dw2-fde.c > +++ b/libgcc/unwind-dw2-fde.c > @@ -67,6 +67,8 @@ static void > init_object (struct object *ob); > > #else > +/* Without fast path frame deregistration must always succeed. */ > +static const int in_shutdown = 0; > > /* The unseen_objects list contains objects that have been registered > but not yet categorized in any way. The seen_objects list has had > Thanks for the patch. I'll let you and Jason decide which style solution is preferred. Thanks, David
Re: [PATCH] Avoid depending on destructor order
On Fri, Sep 23, 2022 at 9:38 AM Thomas Neumann wrote: > > This patch broke bootstrap on AIX and probably other targets. > > > > #ifdef ATOMIC_FDE_FAST_PATH > > #include "unwind-dw2-btree.h" > > > > static struct btree registered_frames; > > static bool in_shutdown; > > ... > > #else > > > > in_shutdown only is defined for ATOMIC_FDE_FAST_PATH but used in code / > > asserts not protected by that macro. > > > >gcc_assert (in_shutdown || ob); > >return (void *) ob; > > } > > I am sorry for that, I did not consider that my test machines all use > the fast path. > > I think the problem can be fixed by the trivial patch below, I will > commit that after I have tested builds both with and without fast path. > > Best > > Thomas > > > diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c > index d237179f4ea..d6e347c5481 100644 > --- a/libgcc/unwind-dw2-fde.c > +++ b/libgcc/unwind-dw2-fde.c > @@ -67,6 +67,8 @@ static void > init_object (struct object *ob); > > #else > +/* Without fast path frame lookup must always succeed */ > +static const bool in_shutdown = false; > > /* The unseen_objects list contains objects that have been registered > but not yet categorized in any way. The seen_objects list has had > I tried the patch but it still failed because the type name "bool" is not known. This patch is the only use of "bool" in the libgcc source code, which is C, not C++. Thanks, David
Re: [PATCH] Avoid depending on destructor order
On Fri, Sep 23, 2022 at 9:38 AM Thomas Neumann wrote: > > This patch broke bootstrap on AIX and probably other targets. > > > > #ifdef ATOMIC_FDE_FAST_PATH > > #include "unwind-dw2-btree.h" > > > > static struct btree registered_frames; > > static bool in_shutdown; > > ... > > #else > > > > in_shutdown only is defined for ATOMIC_FDE_FAST_PATH but used in code / > > asserts not protected by that macro. > > > >gcc_assert (in_shutdown || ob); > >return (void *) ob; > > } > > I am sorry for that, I did not consider that my test machines all use > the fast path. > > I think the problem can be fixed by the trivial patch below, I will > commit that after I have tested builds both with and without fast path. > > Best > > Thomas > > > diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c > index d237179f4ea..d6e347c5481 100644 > --- a/libgcc/unwind-dw2-fde.c > +++ b/libgcc/unwind-dw2-fde.c > @@ -67,6 +67,8 @@ static void > init_object (struct object *ob); > > #else > +/* Without fast path frame lookup must always succeed */ > The comment should end with full stop and two spaces. > +static const bool in_shutdown = false; > I'll let Jason or others decide if this is the right solution. It seems that in_shutdown also could be declared outside the #ifdef and initialized as "false". Thanks, David > > /* The unseen_objects list contains objects that have been registered > but not yet categorized in any way. The seen_objects list has had >
Re: [PATCH] Restore XCOFF for DWARF on AIX.
On Wed, Sep 7, 2022 at 7:45 AM Martin Liška wrote: > Hi. > > The patch restores DWARF support for AIX target where XCOFF file container > is used. > Verified before and after the patch, gcc119 machine (AIX) could not build > any run-time library, > now it can. > > Ready to be installed? > Thanks, > Martin > > PR bootstrap/106855 > > gcc/ChangeLog: > > * collect2.cc (scan_prog_file): Restore if XCOFF_DEBUGGING_INFO. > * config/rs6000/rs6000.cc (rs6000_option_override_internal): > Restore usage of XCOFF_DEBUGGING_INFO. > * config/rs6000/xcoff.h (XCOFF_DEBUGGING_INFO): Restore. > * dwarf2asm.cc (XCOFF_DEBUGGING_INFO): Restore support for > XCOFF_DEBUGGING_INFO. > (dw2_asm_output_nstring): Likewise. > (USE_LINKONCE_INDIRECT): Likewise. > * dwarf2out.cc (XCOFF_DEBUGGING_INFO): Likewise. > (HAVE_XCOFF_DWARF_EXTRAS): Likewise. > (output_fde): Likewise. > (output_call_frame_info): Likewise. > (have_macinfo): Likewise. > (add_AT_loc_list): Likewise. > (add_AT_view_list): Likewise. > (output_compilation_unit_header): Likewise. > (output_pubnames): Likewise. > (output_aranges): Likewise. > (output_line_info): Likewise. > (output_macinfo): Likewise. > (dwarf2out_finish): Likewise. > (dwarf2out_early_finish): Likewise. > Hi, Martin Thanks for the quick patch to fix this. This restores bootstrap, but does not completely restore functionality. This patch did not restore the gcc/configure test for AIX assembler XCOFF feature support that defines HAVE_XCOFF_DWARF_EXTRAS. That part of the removal patch also needs to be reverted. Thanks, David
Re: [PATCH 1/3] STABS: remove -gstabs and -gxcoff functionality
* dwarf2out.cc (XCOFF_DEBUGGING_INFO): Likewise. (HAVE_XCOFF_DWARF_EXTRAS): Likewise. (output_fde): Likewise. (output_call_frame_info): Likewise. (have_macinfo): Likewise. (add_AT_loc_list): Likewise. (add_AT_view_list): Likewise. (output_compilation_unit_header): Likewise. (output_pubnames): Likewise. (output_aranges): Likewise. (output_line_info): Likewise. (output_macinfo): Likewise. (dwarf2out_finish): Likewise. (dwarf2out_early_finish): Likewise. These changes are not correct and break AIX bootstrap. Those changes are not related to stabs support. We agreed to remove stabs and XCOFF stabs support, not GCC DWARF debugging support for AIX. Also * configure: Regenerate. Likewise. * configure.ac: Likewise. does not list that tests for HAVE_XCOFF_DWARF_EXTRAS was removed, so the ChangeLog was not accurate. Again, that test is required for AIX is not part of stabs support. Please revert this change so that AIX can continue to be bootstrapped and tested, and we can work together to test a corrected patch. Thanks, David On Tue, Sep 6, 2022 at 12:31 PM David Edelsohn wrote: > I fully support the plan to remove stabs support, but this patch broke > bootstrap on AIX. It seems rather bad policy to remove support for a > feature without ensuring that the removal does not negatively impact the > targets touched by the patch. I should have been explicitly copied on > these patches and I should have been asked to test the patches before they > were installed, if for no other reason than politeness and consideration. > > Thanks, David > > >
Re: [PATCH 1/3] STABS: remove -gstabs and -gxcoff functionality
I fully support the plan to remove stabs support, but this patch broke bootstrap on AIX. It seems rather bad policy to remove support for a feature without ensuring that the removal does not negatively impact the targets touched by the patch. I should have been explicitly copied on these patches and I should have been asked to test the patches before they were installed, if for no other reason than politeness and consideration. Thanks, David
Re: [PATCH, rs6000] Change insn condition from TARGET_64BIT to TARGET_POWERPC64 for VSX scalar extract/insert instructions
On Thu, Aug 25, 2022 at 10:42 PM HAO CHEN GUI wrote:
>
> Hi David,
>
> On 25/8/2022 下午 10:01, David Edelsohn wrote:
> > On Thu, Aug 25, 2022 at 1:22 AM Kewen.Lin wrote:
> >>
> >> on 2022/8/25 11:37, HAO CHEN GUI wrote:
> >>> Hi,
> >>>
> >>> On 24/8/2022 下午 1:24, Kewen.Lin wrote:
> >>>> Could you try to test with dg-options "-mdejagnu-cpu=power9 -mpowerpc64"
> >>>> all the time, but still
> >>>> having that has_arch_ppc64 effective target on aix?
> >>>>
> >>>> I'd expect has_arch_ppc64 check to fail on aix 32bit, the error will not
> >>>> be a problem (turning
> >>>> into an UNSUPPORTED then)?
> >>>
> >>> I tested it on AIX. "has_arch_ppc64" fails with dg-options
> >>> "-mdejagnu-cpu=power9 -mpowerpc64" on
> >>> 32-bit AIX environment. It works as we expected.
> >>
> >> Nice, thanks for your time on testing.
> >>
> >>>
> >>> Also I found that AIX and Darwin are skipped for bfp test. So in
> >>> testcase, it's no need to care
> >>> about them. Not sure if it's intention.
> >>>
> >>> In bfp.exp
> >>>
> >>> # Exit immediately if this isn't a PowerPC target or if the target is
> >>> # aix or Darwin.
> >>> if { (![istarget powerpc*-*-*] && ![istarget rs6000-*-*])
> >>> || [istarget "powerpc*-*-aix*"]
> >>> || [istarget "powerpc*-*-darwin*"] } then {
> >>> return
> >>> }
> >>
> >> I can't find a hint about why we wanted to disable bfp testing on aix, it
> >> looks like a overkill to me.
> >>
> >> Could you help to further test if all test cases in this small bucket
> >> available on aix?
> >>
> >> Maybe it can give us some evidences on why it's intentional or not.
> >>
> >> Hi David & Segher,
> >>
> >> Do you have some insights on this?
> >
> > AIX (and Darwin) are not Linux and not ELF. There is no support for
> > BPF. All of the tests fail, so they are skipped.
>
> Thanks so much for your info.
>
> Here are test results on P7 AIX7.1. I tested all scalar-extract-sig-* and
> scalar-insert-exp-* cases in
> "testsuite/powerpc/bfp" fold. All compiling cases pass except those use
> __ieee128. The runnable cases
> fail as expected. p9vector is not supported on P7 servers.
>
> So the __ieee128 blocks Binary floating-point on AIX?
AIX does not support IEEE 128 bit floating point, only the IBM
double-double format. Also GCC for AIX does not recognize the
attributes and options for other formats, although there are some
patches from Mike to address that.
Thanks, David
Re: [PATCH, rs6000] Change insn condition from TARGET_64BIT to TARGET_POWERPC64 for VSX scalar extract/insert instructions
On Thu, Aug 25, 2022 at 1:22 AM Kewen.Lin wrote:
>
> on 2022/8/25 11:37, HAO CHEN GUI wrote:
> > Hi,
> >
> > On 24/8/2022 下午 1:24, Kewen.Lin wrote:
> >> Could you try to test with dg-options "-mdejagnu-cpu=power9 -mpowerpc64"
> >> all the time, but still
> >> having that has_arch_ppc64 effective target on aix?
> >>
> >> I'd expect has_arch_ppc64 check to fail on aix 32bit, the error will not
> >> be a problem (turning
> >> into an UNSUPPORTED then)?
> >
> > I tested it on AIX. "has_arch_ppc64" fails with dg-options
> > "-mdejagnu-cpu=power9 -mpowerpc64" on
> > 32-bit AIX environment. It works as we expected.
>
> Nice, thanks for your time on testing.
>
> >
> > Also I found that AIX and Darwin are skipped for bfp test. So in testcase,
> > it's no need to care
> > about them. Not sure if it's intention.
> >
> > In bfp.exp
> >
> > # Exit immediately if this isn't a PowerPC target or if the target is
> > # aix or Darwin.
> > if { (![istarget powerpc*-*-*] && ![istarget rs6000-*-*])
> > || [istarget "powerpc*-*-aix*"]
> > || [istarget "powerpc*-*-darwin*"] } then {
> > return
> > }
>
> I can't find a hint about why we wanted to disable bfp testing on aix, it
> looks like a overkill to me.
>
> Could you help to further test if all test cases in this small bucket
> available on aix?
>
> Maybe it can give us some evidences on why it's intentional or not.
>
> Hi David & Segher,
>
> Do you have some insights on this?
AIX (and Darwin) are not Linux and not ELF. There is no support for
BPF. All of the tests fail, so they are skipped.
Thanks, David
Re: [PATCH v4, rs6000] Add V1TI into vector comparison expand [PR103316]
On Thu, May 26, 2022 at 1:52 AM Kewen.Lin wrote: > > Hi Haochen, > > on 2022/5/26 13:30, HAO CHEN GUI wrote: > > Kewen, > > Thanks so much for your advice. Just one question about effective-target. > > > > For the test cases, it needs both power10_ok and int128 support. I saw > > some > > existing test cases have these two checks as well. But I wonder if > > power10_ok > > already covers int128 on powerpc targets? Can we save one check then? > > > > Good question, IMHO the checks are orthogonal, it's doable to disable int128 > support by hacking the compiler, the int128 effective-target check then fails > due to missing defined __SIZEOF_INT128__, but power10_ok check isn't able to > catch that, the test case could end up with possible unexpected result without > the explicit int128 check. > > To me, the int128 check is to ensure int128 type is available and the > power10_ok check is to ensure the power10 specific instructions are supported. Does Power10 fully support int128 in 32 bit mode? I would expect no, so the additional test is required. Thanks, David > > BR, > Kewen
Re: [PATCH] testsuite: Add test case for pack/unpack bifs at soft-float [PR105334]
On Wed, Apr 27, 2022 at 8:22 AM Segher Boessenkool
wrote:
>
> Hi!
>
> Thank you for doing this testcase.
>
> On Wed, Apr 27, 2022 at 06:29:07PM +0800, Kewen.Lin wrote:
> > As discussed in PR105334, this patch is to add the test coverage for
> > the two recent fixes r12-8091 and r12-8226 from Segher, aix is skipped
> > since it takes soft-float and long-double-128 incompatible.
> >
> > I noticed the referred test case pack02.c skips if powerpc*-*-darwin*,
> > but it's for do-run and I didn't have one machine to test that triple,
> > so I didn't add that and hope it's unnecessary.
>
> That is the best thing to do if you aren't sure, the Darwin people are
> in a much better position to decide this themselves. Cc:ed Iain.
>
> > Tested on powerpc64-linux-gnu P8, powerpc64le-linux-gnu P9/P10 and
> > powerpc-ibm-aix7.2.0.0.
> >
> > Is ok for trunk?
>
> Okay for trunk, thanks! Please see if David has some more input?
>
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/powerpc/pr105334.c
> > @@ -0,0 +1,31 @@
> > +/* Skip this on aix, since it takes soft-float and long-double-128
> > + incompatible and warns it. */
> > +/* { dg-skip-if "aix long-double-128 soft-float" { powerpc*-*-aix* } } */
> > +/* { dg-options "-mlong-double-128 -msoft-float" } */
>
> Maybe you just want to add "-w" and not skip on AIX, if the test
> generates the correct code anyway? Either way works for me though.
The additional libgcc functions for ibm128 weren't added to the AIX
configuration. And it' not clear that soft-float on AIX is
interesting anyway.
Thanks, David
Re: [PATCH] ppc: testsuite: require target effectively [PR104253]
On Mon, Apr 11, 2022 at 10:53 AM Alexandre Oliva wrote:
>
>
> The testcase was missing dg- before require-effective-target.
>
> While at that, I'm also pruning the excess-error warning I got when
> the test failed to be disabled because of the above. I suppose it
> might be useful for some target variants.
>
> Tested with target powerpc64-wrs-vxworks7r2. Ok to install? Trunk?
> gcc-11? gcc-10?
Okay. This probably counts as obvious.
Thanks, David
>
>
> for gcc/testsuite/ChangeLog
>
> PR target/104253
> * gcc.target/powerpc/pr104253.c: Add missing dg- before
> require-effective-target. Prune warning about -mfloat128
> possibly not being fully supported.
> ---
> gcc/testsuite/gcc.target/powerpc/pr104253.c |3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr104253.c
> b/gcc/testsuite/gcc.target/powerpc/pr104253.c
> index 02049cc978f05..e5f9499b7c881 100644
> --- a/gcc/testsuite/gcc.target/powerpc/pr104253.c
> +++ b/gcc/testsuite/gcc.target/powerpc/pr104253.c
> @@ -6,8 +6,9 @@
> */
>
> /* { dg-do run } */
> -/* { require-effective-target ppc_float128_sw } */
> +/* { dg-require-effective-target ppc_float128_sw } */
> /* { dg-options "-O2 -mvsx -mfloat128" } */
> +/* { dg-prune-output ".-mfloat128. option may not be fully supported" } */
>
> /*
> * PR target/104253
>
>
> --
> Alexandre Oliva, happy hackerhttps://FSFLA.org/blogs/lxo/
>Free Software Activist GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts. Ask me about
Re: [PATCH] rs6000: Skip overload instances with NULL fntype [PR104967]
On Wed, Mar 23, 2022 at 5:33 AM Kewen.Lin wrote: > > Hi, > > As shown in PR104967, for some overload built-in function instance, > if it requires a date type which isn't defined on the target, its > fntype would be initialized as NULL. This patch is to consider > this possibility in function find_instance to avoid ICE. > > Bootstrapped and regtested on powerpc64-linux-gnu P8 and > powerpc64le-linux-gnu P9 and P10. > > Is it ok for trunk? Okay. Thanks, David > > BR, > Kewen > > PR target/104967 > > gcc/ChangeLog: > > * config/rs6000/rs6000-c.cc (find_instance): Skip instances with null > function types. > > --- > gcc/config/rs6000/rs6000-c.cc | 4 > 1 file changed, 4 insertions(+) > > diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc > index 15251efc209..b0f9fce4b54 100644 > --- a/gcc/config/rs6000/rs6000-c.cc > +++ b/gcc/config/rs6000/rs6000-c.cc > @@ -1678,6 +1678,10 @@ find_instance (bool *unsupported_builtin, ovlddata > **instance, > >ovlddata *inst = *instance; >gcc_assert (inst != NULL); > + /* It is possible for an instance to require a data type that isn't > + defined on this target, in which case inst->fntype will be NULL. */ > + if (!inst->fntype) > +return error_mark_node; >tree fntype = rs6000_builtin_info[inst->bifid].fntype; >tree parmtype0 = TREE_VALUE (TYPE_ARG_TYPES (fntype)); >tree parmtype1 = TREE_VALUE (TREE_CHAIN (TYPE_ARG_TYPES (fntype))); > -- > 2.25.1
Re: [PATCH]rs6000: avoid peeking eof after __vector keyword
On Mon, Mar 21, 2022 at 5:13 AM Jiufu Guo wrote:
>
> Hi!
>
> There is a rare corner case: where __vector is followed only with ";"
> and near the end of the file.
>
> Like the case in PR101168:
> using vdbl = __vector double;
> #define BREAK 1
>
> For this case, "__vector double" is not followed by a PP_NAME, it is
> followed by CPP_SEMICOLON and then EOF. In this case, there is no
> more tokens in the file. Then, do not need to continue to parse the
> file.
>
> This patch pass bootstrap and regtest on ppc64 and ppc64le.
This is okay. Maybe a tweak to the comment, see below.
Thanks, David
>
>
> BR,
> Jiufu
>
>
> PR preprocessor/101168
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000-c.cc (rs6000_macro_to_expand):
> Avoid empty identifier.
>
> gcc/testsuite/ChangeLog:
>
> * g++.target/powerpc/pr101168.C: New test.
>
>
> ---
> gcc/config/rs6000/rs6000-c.cc | 4 +++-
> gcc/testsuite/g++.target/powerpc/pr101168.C | 6 ++
> 2 files changed, 9 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/g++.target/powerpc/pr101168.C
>
> diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
> index 3b62b499df2..f8cc7bad812 100644
> --- a/gcc/config/rs6000/rs6000-c.cc
> +++ b/gcc/config/rs6000/rs6000-c.cc
> @@ -282,7 +282,9 @@ rs6000_macro_to_expand (cpp_reader *pfile, const
> cpp_token *tok)
> expand_bool_pixel = __pixel_keyword;
> else if (ident == C_CPP_HASHNODE (__bool_keyword))
> expand_bool_pixel = __bool_keyword;
> - else
> +
> + /* If it needs to check tokens continue. */
Maybe /* If there are more tokens to check. */ ?
> + else if (ident)
> {
> /* Try two tokens down, too. */
> do
> diff --git a/gcc/testsuite/g++.target/powerpc/pr101168.C
> b/gcc/testsuite/g++.target/powerpc/pr101168.C
> new file mode 100644
> index 000..284e77fdc88
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/powerpc/pr101168.C
> @@ -0,0 +1,6 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_altivec_ok } */
> +/* { dg-options "-maltivec" } */
> +
> +using vdbl = __vector double;
> +#define BREAK 1
> --
> 2.25.1
>
Re: [PATCH] libgcc: allow building float128 libraries on FreeBSD
I don't have any objection, but the patch is FreeBSD-specific. You
are sending the patch from the FreeBSD organization, but I don't know
the authority structure within the organization. Andreas Tobler is
the FreeBSD maintainer for GCC, but I don't know his current status.
Thanks, David
On Sun, Feb 20, 2022 at 6:38 PM wrote:
>
> From: Piotr Kubaj
>
> While FreeBSD currently uses 64-bit long double, there should be no
> problem with adding support for float128.
>
> Signed-off-by: Piotr Kubaj
> ---
> libgcc/configure| 22 ++
> libgcc/configure.ac | 11 +++
> 2 files changed, 33 insertions(+)
>
> diff --git a/libgcc/configure b/libgcc/configure
> index 4919a56f518..334d20d1fb1 100755
> --- a/libgcc/configure
> +++ b/libgcc/configure
> @@ -5300,6 +5300,28 @@ fi
> { $as_echo "$as_me:${as_lineno-$LINENO}: result:
> $libgcc_cv_powerpc_3_1_float128_hw" >&5
> $as_echo "$libgcc_cv_powerpc_3_1_float128_hw" >&6; }
>CFLAGS="$saved_CFLAGS"
> +;;
> +powerpc*-*-freebsd*)
> + saved_CFLAGS="$CFLAGS"
> + CFLAGS="$CFLAGS -mabi=altivec -mvsx -mfloat128"
> + { $as_echo "$as_me:${as_lineno-$LINENO}: checking for PowerPC ISA 2.06 to
> build __float128 libraries" >&5
> +$as_echo_n "checking for PowerPC ISA 2.06 to build __float128 libraries... "
> >&6; }
> +if ${libgcc_cv_powerpc_float128+:} false; then :
> + $as_echo_n "(cached) " >&6
> +else
> + cat confdefs.h - <<_ACEOF >conftest.$ac_ext
> +/* end confdefs.h. */
> +vector double dadd (vector double a, vector double b) { return a + b; }
> +_ACEOF
> +if ac_fn_c_try_compile "$LINENO"; then :
> + libgcc_cv_powerpc_float128=yes
> +else
> + libgcc_cv_powerpc_float128=no
> +fi
> +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
> +fi
> +{ $as_echo "$as_me:${as_lineno-$LINENO}: result:
> $libgcc_cv_powerpc_float128" >&5
> +$as_echo "$libgcc_cv_powerpc_float128" >&6; }
> esac
>
> # Collect host-machine-specific information.
> diff --git a/libgcc/configure.ac b/libgcc/configure.ac
> index 13a80b2551b..99ec5d405a4 100644
> --- a/libgcc/configure.ac
> +++ b/libgcc/configure.ac
> @@ -483,6 +483,17 @@ powerpc*-*-linux*)
> [libgcc_cv_powerpc_3_1_float128_hw=yes],
> [libgcc_cv_powerpc_3_1_float128_hw=no])])
>CFLAGS="$saved_CFLAGS"
> +;;
> +powerpc*-*-freebsd*)
> + saved_CFLAGS="$CFLAGS"
> + CFLAGS="$CFLAGS -mabi=altivec -mvsx -mfloat128"
> + AC_CACHE_CHECK([for PowerPC ISA 2.06 to build __float128 libraries],
> + [libgcc_cv_powerpc_float128],
> + [AC_COMPILE_IFELSE(
> +[AC_LANG_SOURCE([vector double dadd (vector double a, vector double b) {
> return a + b; }])],
> +[libgcc_cv_powerpc_float128=yes],
> +[libgcc_cv_powerpc_float128=no])])
> + CFLAGS="$saved_CFLAGS"
> esac
>
> # Collect host-machine-specific information.
> --
> 2.35.1
>
Re: [PATCH v3, rs6000] Enable absolute jump table for PPC AIX and Linux
On Tue, Mar 1, 2022 at 12:41 AM HAO CHEN GUI wrote:
>
> Hi,
>This patch enables absolute jump tables on PPC AIX and Linux. For AIX, the
> jump
> table is placed in data section. For Linux, it is placed in RELRO section when
> relocation is needed.
>
>Bootstrapped and tested on AIX,Linux BE and LE with no regressions. Is
> this okay for trunk?
> Any recommendations? Thanks a lot.
Hi, Haochen
Thanks for working on this patch and for revising it. The patch looks
okay now, but it is not appropriate for the current bug fixing stage
in GCC development -- it needs to wait for GCC Stage 1 later this
Spring.
Also the current code uses the default readonly data section for the
jump table. The new rs6000_xcoff_function_rodata_section follows the
existing simple behavior, which is correct, but it should support
named data sections instead of placing everything in the default
".data" section, similar to the default ELF code. I will work on
that.
Thanks, David
>
> ChangeLog
> 2022-03-01 Haochen Gui
>
> gcc/
> * config/rs6000/aix.h (JUMP_TABLES_IN_TEXT_SECTION): Define.
> * config/rs6000/linux64.h (JUMP_TABLES_IN_TEXT_SECTION): Likewise.
> * config/rs6000/rs6000.cc (rs6000_option_override_internal): Enable
> absolute jump tables for AIX and Linux.
> (rs6000_xcoff_function_rodata_section): Implement.
> * config/rs6000/xcoff.h (TARGET_ASM_FUNCTION_RODATA_SECTION): Define.
>
> gcc/testsuite
> * gcc.target/powerpc/absolute-jump-table-section.c: New.
>
>
> patch.diff
> diff --git a/gcc/config/rs6000/aix.h b/gcc/config/rs6000/aix.h
> index ad3238bf09a..cf0708aa08b 100644
> --- a/gcc/config/rs6000/aix.h
> +++ b/gcc/config/rs6000/aix.h
> @@ -251,9 +251,9 @@
> #define BLOCK_REG_PADDING(MODE, TYPE, FIRST) \
>(!(FIRST) ? PAD_UPWARD : targetm.calls.function_arg_padding (MODE, TYPE))
>
> -/* Indicate that jump tables go in the text section. */
> +/* Indicate that jump tables go in the data section. */
>
> -#define JUMP_TABLES_IN_TEXT_SECTION 1
> +#define JUMP_TABLES_IN_TEXT_SECTION 0
>
> /* Define any extra SPECS that the compiler needs to generate. */
> #undef SUBTARGET_EXTRA_SPECS
> diff --git a/gcc/config/rs6000/linux64.h b/gcc/config/rs6000/linux64.h
> index b2a7afabc73..440e0fde52b 100644
> --- a/gcc/config/rs6000/linux64.h
> +++ b/gcc/config/rs6000/linux64.h
> @@ -237,9 +237,9 @@ extern int dot_symbols;
> #define TARGET_ALIGN_NATURAL 1
> #endif
>
> -/* Indicate that jump tables go in the text section. */
> +/* Indicate that jump tables go in the rodata or RELRO section. */
> #undef JUMP_TABLES_IN_TEXT_SECTION
> -#define JUMP_TABLES_IN_TEXT_SECTION TARGET_64BIT
> +#define JUMP_TABLES_IN_TEXT_SECTION 0
>
> /* The linux ppc64 ABI isn't explicit on whether aggregates smaller
> than a doubleword should be padded upward or downward. You could
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index bc3ef0721a4..07f78d3a05b 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -4954,6 +4954,10 @@ rs6000_option_override_internal (bool global_init_p)
> warning (0, "%qs is deprecated and not recommended in any circumstances",
> "-mno-speculate-indirect-jumps");
>
> + /* Enable absolute jump tables for AIX and Linux. */
> + if (DEFAULT_ABI == ABI_AIX || DEFAULT_ABI == ABI_ELFv2)
> +rs6000_relative_jumptables = 0;
> +
>return ret;
> }
>
> @@ -21419,6 +21423,16 @@ rs6000_xcoff_visibility (tree decl)
>enum symbol_visibility vis = DECL_VISIBILITY (decl);
>return visibility_types[vis];
> }
> +
> +static section *
> +rs6000_xcoff_function_rodata_section (tree decl ATTRIBUTE_UNUSED,
> + bool relocatable)
> +{
> + if (relocatable)
> +return data_section;
> + else
> +return readonly_data_section;
> +}
> #endif
>
>
> diff --git a/gcc/config/rs6000/xcoff.h b/gcc/config/rs6000/xcoff.h
> index cd0f99cb9c6..0dacd86eed9 100644
> --- a/gcc/config/rs6000/xcoff.h
> +++ b/gcc/config/rs6000/xcoff.h
> @@ -98,7 +98,7 @@
> #define TARGET_ASM_SELECT_SECTION rs6000_xcoff_select_section
> #define TARGET_ASM_SELECT_RTX_SECTION rs6000_xcoff_select_rtx_section
> #define TARGET_ASM_UNIQUE_SECTION rs6000_xcoff_unique_section
> -#define TARGET_ASM_FUNCTION_RODATA_SECTION default_no_function_rodata_section
> +#define TARGET_ASM_FUNCTION_RODATA_SECTION
> rs6000_xcoff_function_rodata_section
> #define TARGET_STRIP_NAME_ENCODING rs6000_xcoff_strip_name_encoding
> #define TARGET_SECTION_TYPE_FLAGS rs6000_xcoff_section_type_flags
> #ifdef HAVE_AS_TLS
> diff --git a/gcc/testsuite/gcc.target/powerpc/absolute-jump-table-section.c
> b/gcc/testsuite/gcc.target/powerpc/absolute-jump-table-section.c
> new file mode 100644
> index 000..688a6f42836
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/absolute-jump-table-section.c
> @@ -0,0 +1,29 @@
> +/* { dg-do compile { target { *-*-aix* || *-*-linux* } } } */
> +/* {
Re: [PATCH, 11 backport] rs6000: Fix LE code gen for vec_cnt[lt]z_lsbb [PR95082]
On Thu, Feb 10, 2022 at 4:17 PM Bill Schmidt wrote:
>
> Hi!
>
> On 2/10/22 2:50 PM, Segher Boessenkool wrote:
> > On Thu, Feb 10, 2022 at 12:22:28PM -0600, Bill Schmidt wrote:
> >> This is a backport from mainline 3f30f2d1dbb3228b8468b26239fe60c2974ce2ac.
> >> These built-ins were misimplemented as always having big-endian semantics.
> >>
> >> Because the built-in infrastructure has changed, the modifications to the
> >> source are different but achieve the same purpose. The modifications to
> >> the test suite are identical (after fixing the issue with -mbig that David
> >> pointed out with the original patch).
> >> /* 1 argument vector functions added in ISA 3.0 (power9). */
> >> -BU_P9V_AV_1 (VCLZLSBB_V16QI, "vclzlsbb_v16qi", CONST,
> >> vclzlsbb_v16qi)
> >> -BU_P9V_AV_1 (VCLZLSBB_V8HI, "vclzlsbb_v8hi",CONST, vclzlsbb_v8hi)
> >> -BU_P9V_AV_1 (VCLZLSBB_V4SI, "vclzlsbb_v4si",CONST, vclzlsbb_v4si)
> >> -BU_P9V_AV_1 (VCTZLSBB_V16QI, "vctzlsbb_v16qi", CONST,
> >> vctzlsbb_v16qi)
> >> -BU_P9V_AV_1 (VCTZLSBB_V8HI, "vctzlsbb_v8hi",CONST, vctzlsbb_v8hi)
> >> -BU_P9V_AV_1 (VCTZLSBB_V4SI, "vctzlsbb_v4si",CONST, vctzlsbb_v4si)
> >> +BU_P9V_AV_1 (VCLZLSBB_V16QI, "vclzlsbb_v16qi", CONST,
> >> vctzlsbb_v16qi)
> >> +BU_P9V_AV_1 (VCLZLSBB_V8HI, "vclzlsbb_v8hi",CONST, vctzlsbb_v8hi)
> >> +BU_P9V_AV_1 (VCLZLSBB_V4SI, "vclzlsbb_v4si",CONST, vctzlsbb_v4si)
> >> +BU_P9V_AV_1 (VCTZLSBB_V16QI, "vctzlsbb_v16qi", CONST,
> >> vclzlsbb_v16qi)
> >> +BU_P9V_AV_1 (VCTZLSBB_V8HI, "vctzlsbb_v8hi",CONST, vclzlsbb_v8hi)
> >> +BU_P9V_AV_1 (VCTZLSBB_V4SI, "vctzlsbb_v4si",CONST, vclzlsbb_v4si)
> > Please change the default to be equal to the builtin name, so, the BE
> > version. We do that everywhere else as well, and it makes a lot more
> > sense (since everything in Power has BE numbering).
> >
> > The trunk version has this correct afaics?
>
> No, trunk has this, for example:
>
> const signed int __builtin_altivec_vclzlsbb_v16qi (vsc);
> VCLZLSBB_V16QI vctzlsbb_v16qi {endian}
>
> So the backport matches what is on trunk.
>
> Throughout the new builtin infrastructure, the defaults are set for
> little-endian, and the "endian" flag changes behavior for big-endian.
>
> >
> >> --- a/gcc/testsuite/gcc.target/powerpc/vsu/vec-cntlz-lsbb-0.c
> >> +++ b/gcc/testsuite/gcc.target/powerpc/vsu/vec-cntlz-lsbb-0.c
> >> @@ -1,6 +1,7 @@
> >> /* { dg-do compile { target { powerpc*-*-* } } } */
> > (Delete the redundant target clause when modifying any testcase, please).
>
> Okay.
> >
> >> /* { dg-require-effective-target powerpc_p9vector_ok } */
> >> /* { dg-options "-mdejagnu-cpu=power9" } */
> >> +/* { dg-additional-options "-mbig" { target powerpc64le-*-* } } */
> > You don't need the target clause, if it already is BE by default it does
> > not do anything to add it redundantly.
> >
> > But this is wrong anyway: the name of the target triple does not say
> > whether we are BE or LE. Instead you should use the be or le selectors.
> > But again, just add -mbig always.
>
> This was added by David Edelsohn to the trunk version of the patch, because
> -mbig actually is not supported on all subtargets. (I found that quite
> surprising also.) Apparently this doesn't work on AIX, for example. But
> -mlittle works everywhere. Go figure.
-mbig/-mlittle only applies to Linux, not AIX and not Darwin.
I changed the BE testcases to add "-mbig" for little endian default
targets because the compiler implicitly should be operating in big
endian mode for other targets and the testcases should succeed.
For the LE testcases, I changed the target selector to
"powrpc*-*-linux*" because that is the only PowerPC target that can
operate as little endian. I could not find a generic "le" target
selector. powerpc*-*-linux* understands "-mlittle", so I left the
dg-options clause because there is no need to separate out "-mlittle"
for that subset of PowerPC targets.
Thanks, David
>
> That's something that should be fixed, I guess, but it's orthogonal
> to this patch.
>
> Thanks!
> Bill
>
> >
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/powerpc/vsu/vec-cntlz-lsbb-3.c
> >> @@ -0,0 +1,15 @@
> >> +/* { dg-do compile { target { powerpc*-*-* } } } */
> >> +/* { dg-require-effective-target powerpc_p9vector_ok } */
> >> +/* { dg-options "-mdejagnu-cpu=power9 -mlittle" } */
> > And here you do it correctly :-)
> >
> > Okay with those fixes (all happen a few times). Thanks!
> >
> >
> > Segher
Re: [PATCH] rs6000: Fix up vspltis_shifted [PR102140]
On Tue, Feb 8, 2022 at 12:25 PM Jakub Jelinek wrote:
>
> Hi!
>
> The following testcase ICEs, because
> (const_vector:V4SI [
> (const_int 0 [0]) repeated x3
> (const_int -2147483648 [0x8000])
> ])
> is recognized as valid easy_vector_constant in between split1 pass and
> end of RA.
> The problem is that such constants need to be split, and the only
> splitter for that is:
> (define_split
> [(set (match_operand:VM 0 "altivec_register_operand")
> (match_operand:VM 1 "easy_vector_constant_vsldoi"))]
> "VECTOR_UNIT_ALTIVEC_OR_VSX_P (mode) && can_create_pseudo_p ()"
> There is only a single splitting pass before RA, so after that finishes,
> if something gets matched in between that and end of RA (after that
> can_create_pseudo_p () would be no longer true), it will never be
> successfully split and we ICE at final.cc time or earlier.
>
> The i386 backend (and a few others) already use
> (cfun->curr_properties & PROP_rtl_split_insns)
> as a test for split1 pass finished, so that some insns that should be split
> during split1 and shouldn't be matched afterwards are properly guarded.
>
> So, the following patch does that for vspltis_shifted too.
>
> Bootstrapped/regtested on powerpc64le-linux, ok for trunk?
Okay.
Thanks, David
>
> 2022-02-08 Jakub Jelinek
>
> PR target/102140
> * config/rs6000/rs6000.cc (vspltis_shifted): Return false also if
> split1 pass has finished already.
>
> * gcc.dg/pr102140.c: New test.
>
> --- gcc/config/rs6000/rs6000.cc.jj 2022-02-07 17:38:20.873123915 +0100
> +++ gcc/config/rs6000/rs6000.cc 2022-02-08 14:15:31.619505410 +0100
> @@ -6257,8 +6257,11 @@ vspltis_shifted (rtx op)
> return false;
>
>/* We need to create pseudo registers to do the shift, so don't recognize
> - shift vector constants after reload. */
> - if (!can_create_pseudo_p ())
> + shift vector constants after reload. Don't match it even before RA
> + after split1 is done, because there won't be further splitting pass
> + before RA to do the splitting. */
> + if (!can_create_pseudo_p ()
> + || (cfun->curr_properties & PROP_rtl_split_insns))
> return false;
>
>nunits = GET_MODE_NUNITS (mode);
> --- gcc/testsuite/gcc.dg/pr102140.c.jj 2022-02-08 14:24:25.839041166 +0100
> +++ gcc/testsuite/gcc.dg/pr102140.c 2022-02-08 14:24:03.038359745 +0100
> @@ -0,0 +1,23 @@
> +/* PR target/102140 */
> +/* { dg-do compile { target int128 } } */
> +/* { dg-options "-Og -fipa-cp -fno-tree-ccp -fno-tree-ter -Wno-psabi" } */
> +
> +typedef int __attribute__((__vector_size__ (64))) U;
> +typedef __int128 __attribute__((__vector_size__ (64))) V;
> +
> +int a, b;
> +
> +static void
> +bar (char c, V v)
> +{
> + v *= c;
> + U u = a + (U) v;
> + (union { U b; }) { u };
> + b = 0;
> +}
> +
> +void
> +foo (void)
> +{
> + bar (1, (V){((__int128) 9223372036854775808ULL) << 64});
> +}
>
> Jakub
>
Re: [PATCH] testsuite: Fix up testsuite/gcc.c-torture/execute/builtins/lib/chk.c for powerpc [PR104380]
On Mon, Feb 7, 2022 at 8:20 AM Jakub Jelinek wrote: > > On Fri, Feb 04, 2022 at 12:00:57PM -0500, David Edelsohn via Gcc-patches > wrote: > > > The following testcase FAILs when configured with > > > --with-long-double-format=ieee . Only happens in the -std=c* modes, not > > > the > > > GNU modes; while the glibc headers have __asm redirects of > > > vsnprintf and __vsnprinf_chk to __vsnprintfieee128 and > > > __vsnprintf_chkieee128, the vsnprintf fortification extern inline > > > gnu_inline > > > always_inline wrapper calls __builtin_vsnprintf_chk and we actually emit > > > a call to __vsnprinf_chk (i.e. with IBM extended long double) instead of > > > __vsnprintf_chkieee128. > > > > > > rs6000_mangle_decl_assembler_name already had cases for *printf and > > > *scanf, > > > so this just adds another case for *printf_chk. *scanf_chk doesn't exist. > > > __ prefixing isn't done because *printf_chk already starts with __. > > > > > > Bootstrapped/regtested on powerpc64le-linux, ok for trunk? > > > > Okay. > > Unfortunately, while I've tested the testcase also with -mabi=ieeelongdouble > by hand, the full bootstrap/regtest was on GCCFarm where glibc is too old > to test with --with-long-double-format=ieee. > I've done full bootstrap/regtest with that option during the weekend and > the patch regressed: > FAIL: gcc.c-torture/execute/builtins/snprintf-chk.c execution, -O1 > FAIL: gcc.c-torture/execute/builtins/snprintf-chk.c execution, -O2 > FAIL: gcc.c-torture/execute/builtins/snprintf-chk.c execution, -O2 -flto > -fno-use-linker-plugin -flto-partition=none > FAIL: gcc.c-torture/execute/builtins/snprintf-chk.c execution, -O2 -flto > -fuse-linker-plugin -fno-fat-lto-objects > FAIL: gcc.c-torture/execute/builtins/snprintf-chk.c execution, -O3 > -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions > FAIL: gcc.c-torture/execute/builtins/snprintf-chk.c execution, -O3 -g > FAIL: gcc.c-torture/execute/builtins/snprintf-chk.c execution, -Og -g > FAIL: gcc.c-torture/execute/builtins/snprintf-chk.c execution, -Os > FAIL: gcc.c-torture/execute/builtins/sprintf-chk.c execution, -O1 > FAIL: gcc.c-torture/execute/builtins/sprintf-chk.c execution, -O2 > FAIL: gcc.c-torture/execute/builtins/sprintf-chk.c execution, -O2 -flto > -fno-use-linker-plugin -flto-partition=none > FAIL: gcc.c-torture/execute/builtins/sprintf-chk.c execution, -O2 -flto > -fuse-linker-plugin -fno-fat-lto-objects > FAIL: gcc.c-torture/execute/builtins/sprintf-chk.c execution, -O3 > -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions > FAIL: gcc.c-torture/execute/builtins/sprintf-chk.c execution, -O3 -g > FAIL: gcc.c-torture/execute/builtins/sprintf-chk.c execution, -Og -g > FAIL: gcc.c-torture/execute/builtins/sprintf-chk.c execution, -Os > FAIL: gcc.c-torture/execute/builtins/vsnprintf-chk.c execution, -O1 > FAIL: gcc.c-torture/execute/builtins/vsnprintf-chk.c execution, -O2 > FAIL: gcc.c-torture/execute/builtins/vsnprintf-chk.c execution, -O2 -flto > -fno-use-linker-plugin -flto-partition=none > FAIL: gcc.c-torture/execute/builtins/vsnprintf-chk.c execution, -O2 -flto > -fuse-linker-plugin -fno-fat-lto-objects > FAIL: gcc.c-torture/execute/builtins/vsnprintf-chk.c execution, -O3 > -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions > FAIL: gcc.c-torture/execute/builtins/vsnprintf-chk.c execution, -O3 -g > FAIL: gcc.c-torture/execute/builtins/vsnprintf-chk.c execution, -Og -g > FAIL: gcc.c-torture/execute/builtins/vsnprintf-chk.c execution, -Os > FAIL: gcc.c-torture/execute/builtins/vsprintf-chk.c execution, -O1 > FAIL: gcc.c-torture/execute/builtins/vsprintf-chk.c execution, -O2 > FAIL: gcc.c-torture/execute/builtins/vsprintf-chk.c execution, -O2 -flto > -fno-use-linker-plugin -flto-partition=none > FAIL: gcc.c-torture/execute/builtins/vsprintf-chk.c execution, -O2 -flto > -fuse-linker-plugin -fno-fat-lto-objects > FAIL: gcc.c-torture/execute/builtins/vsprintf-chk.c execution, -O3 > -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions > FAIL: gcc.c-torture/execute/builtins/vsprintf-chk.c execution, -O3 -g > FAIL: gcc.c-torture/execute/builtins/vsprintf-chk.c execution, -Og -g > FAIL: gcc.c-torture/execute/builtins/vsprintf-chk.c execution, -Os > > The problem is that the execute/builtins/ testsuite wants to override some > of the library functions and with the change we (correctly) call > __*printf_chkieee128 and so lib/chk.c is no longer called but the glibc > APIs are. > > The following patch fixes it. > > Tested on powerpc64le-linux, ok for trunk? Okay. Tha
