Re: [patch, libfortran] Fix thead sanitizer issue with libgfortran
On Fri, Sep 29, 2017 at 9:53 PM, Thomas Koenig wrote: > Am 29.09.2017 um 10:03 schrieb Janne Blomqvist: > >> >> 1) I'm confused why fbuf_destroy is modified. The fbuf structure is >> part of gfc_unit, and should be accessed with the same locking rules >> as the rest of the gfc_unit components. When closing the unit, I think >> the same should apply here, no? > > > It is to avoid a data race for > > program main > use omp_lib > !$OMP PARALLEL NUM_THREADS(2) > call file_open(OMP_get_thread_num()) > !$OMP END PARALLEL > contains > recursive subroutine file_open(i) > integer :: i > integer :: nunit > nunit = i + 20 > write (nunit,*) 'asdf' > end subroutine file_open > end program main > > which leads to > > Read of size 4 at 0x7b58ff30 by main thread (mutexes: write M16): > #0 close_unit_1 ../../../trunk/libgfortran/io/unit.c:699 > (libgfortran.so.5+0x00283ba6) > #1 _gfortrani_close_units ../../../trunk/libgfortran/io/unit.c:767 > (libgfortran.so.5+0x00283f59) > #2 cleanup ../../../trunk/libgfortran/runtime/main.c:113 > (libgfortran.so.5+0x0001fe22) > #3 _dl_fini (ld-linux-x86-64.so.2+0xfb62) > > Previous write of size 4 at 0x7b58ff30 by thread T1 (mutexes: write > M17): > #0 finalize_transfer ../../../trunk/libgfortran/io/transfer.c:3934 > (libgfortran.so.5+0x00281aa1) > #1 _gfortran_st_write_done ../../../trunk/libgfortran/io/transfer.c:4125 > (libgfortran.so.5+0x00281e35) > #2 file_open.3528 (a.out+0x00400c1b) > #3 MAIN__._omp_fn.0 (a.out+0x00400d27) > #4 gomp_thread_start ../../../trunk/libgomp/team.c:120 > (libgomp.so.1+0x0001756f) > > Note that not all problems with implicit close at the end are resolved > so far, but that is a different problem. I'm still confused. If gfc_unit->fbuf needs to be protected by holding gfc_unit->lock when closing, surely the same applies to other gfc_units fields as well? How if gfc_unit->fbuf special? AFAICS it isn't.. -- Janne Blomqvist
[PATCH][PR 81376] Remove unnecessary float casts in comparisons
Hi all, (Previous mail was sent with spurious HTML, sorry!) This patch gets rid of float casts in comparisons when all values of casted integral type are exactly representable by the float type (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81376). Bootstrapped and regtested on x64_64. Ok to commit? -Y pr81376-1.patch Description: Binary data
Re: [PATCH gfortran v2] PR 61450: ICE in gfc_global_used()
Hi Dominique, The patch is OK for trunk. Thanks! Thomas
[PATCH] x32: Add and use libgnarl/s-taprop__x32.adb
s-taprop.adb failed to compile for x32: s-taprop.adb:341:29: operator for type "System.Linux.time_t" is not directly visible s-taprop.adb:341:29: add with_clause and use_clause for "Linux" which is caused by 2017-09-25 Doug Rupp * libgnarl/s-taprop__linux.adb (Base_Monotonic_Clock): New variable. (Compute_Base_Monotonic_Clock): New function. (Timed_Sleep): Adjust to use Base_Monotonic_Clock. (Timed_Delay): Likewise. (Monotonic_Clock): Likewise. * s-oscons-tmplt.c (CLOCK_MONOTONIC): Use on Linux. This patch adds and uses libgnarl/s-taprop__x32.adb for x32, which is similar to libgnarl/s-taprop__linux.adb, but uses System.Linux.time_t. OK for trunk? H.J. * gcc-interface/Makefile.in: Replace libgnarl/s-taprop__linux.adb with libgnarl/s-taprop__x32.adb for x32. * libgnarl/s-taprop__x32.adb: New file. --- gcc/ada/gcc-interface/Makefile.in |2 +- gcc/ada/libgnarl/s-taprop__x32.adb | 1752 2 files changed, 1753 insertions(+), 1 deletion(-) create mode 100644 gcc/ada/libgnarl/s-taprop__x32.adb diff --git a/gcc/ada/gcc-interface/Makefile.in b/gcc/ada/gcc-interface/Makefile.in index 2fa47caa547..d8899897481 100644 --- a/gcc/ada/gcc-interface/Makefile.in +++ b/gcc/ada/gcc-interface/Makefile.in @@ -1829,7 +1829,7 @@ ifeq ($(strip $(filter-out %x32 linux%,$(target_cpu) $(target_os))),) s-osinte.adshttp://www.gnu.org/licenses/>. -- +-- -- +-- GNARL was developed by the GNARL team at Florida State University. -- +-- Extensive contributions were provided by Ada Core Technologies, Inc. -- +-- -- +-- + +-- This is a GNU/Linux/x32 (GNU/LinuxThreads) version of this package + +-- This package contains all the GNULL primitives that interface directly with +-- the underlying OS. + +pragma Polling (Off); +-- Turn off polling, we do not want ATC polling to take place during tasking +-- operations. It causes infinite loops and other problems. + +with Interfaces.C; use Interfaces; +use type Interfaces.C.int; +use type Interfaces.C.long; + +with System.Task_Info; +with System.Tasking.Debug; +with System.Interrupt_Management; +with System.OS_Constants; +with System.OS_Primitives; +with System.Multiprocessors; +with System.Linux; + +with System.Soft_Links; +-- We use System.Soft_Links instead of System.Tasking.Initialization +-- because the later is a higher level package that we shouldn't depend on. +-- For example when using the restricted run time, it is replaced by +-- System.Tasking.Restricted.Stages. + +package body System.Task_Primitives.Operations is + + package OSC renames System.OS_Constants; + package SSL renames System.Soft_Links; + + use System.Tasking.Debug; + use System.Tasking; + use System.OS_Interface; + use System.Parameters; + use System.OS_Primitives; + use System.Task_Info; + + + -- Local Data -- + + + -- The followings are logically constants, but need to be initialized + -- at run time. + + Single_RTS_Lock : aliased RTS_Lock; + -- This is a lock to allow only one thread of control in the RTS at + -- a time; it is used to execute in mutual exclusion from all other tasks. + -- Used mainly in Single_Lock mode, but also to protect All_Tasks_List + + Environment_Task_Id : Task_Id; + -- A variable to hold Task_Id for the environment task + + Unblocked_Signal_Mask : aliased sigset_t; + -- The set of signals that should be unblocked in all tasks + + -- The followings are internal configuration constants needed + + Next_Serial_Number : Task_Serial_Number := 100; + -- We start at 100 (reserve some special values for using in error checks) + + Time_Slice_Val : Integer; + pragma Import (C, Time_Slice_Val, "__gl_time_slice_val"); + + Dispatching_Policy : Character; + pragma Import (C, Dispatching_Policy, "__gl_task_dispatching_policy"); + + Locking_Policy : Character; + pragma Import (C, Locking_Policy, "__gl_locking_policy"); + + Foreign_Task_Elaborated : aliased Boolean := True; + -- Used to identified fake tasks (i.e., non-Ada Threads) + + Use_Alternate_Stack : constant Boolean := Alternate_Stack_Size /= 0; + -- Whether to use an alternate signal stack for stack overflows + + Abort_Handler_Installed : Boolean := False; + -- True if a handler for the abort signal is installed + + Null_Thread_Id : constant pthread_t := pthread_t'Last; + -- Constant to indicate that the thread identifier has not yet been + -- initialized. + + Base_Monotonic_Clock : Duration := 0.0; + + + -- Local Packages -- + + + package Specific is + + procedure Initi
Re: [patch, libfortran] Fix thead sanitizer issue with libgfortran
Hi,
I think this might be a false positive tsan warning.
Maybe tsan does not see why a desctructor function
can not execute before the last detached thread
terminates.
I created a small test case that receives a warning
which is similar to the one you tried to fix with your
patch:
cat test.c
#include
#include
int test;
static void *
t1(void* p)
{
printf("t1\n");
test = 1;
return NULL;
}
static void __attribute__((destructor))
t2 (void)
{
test = 2;
printf("t2\n");
}
int
main()
{
pthread_t t;
pthread_attr_t a;
pthread_attr_init(&a);
pthread_attr_setdetachstate(&a, PTHREAD_CREATE_DETACHED);
pthread_create(&t, &a, t1, NULL);
pthread_attr_destroy(&a);
return 0;
}
gcc -Wall -fsanitize=thread test.c
./a.out
t1
t2
==
WARNING: ThreadSanitizer: data race (pid=3564)
Write of size 4 at 0x0060107c by thread T1:
#0 t1 (a.out+0x0040088e)
Previous write of size 4 at 0x0060107c by main thread:
#0 t2 (a.out+0x004008c6)
#1 (ld-linux-x86-64.so.2+0x000108d9)
Location is global 'test' of size 4 at 0x0060107c
(a.out+0x0060107c)
Thread T1 (tid=3566, running) created by main thread at:
#0 pthread_create
../../../../gcc-trunk/libsanitizer/tsan/tsan_interceptors.cc:900
(libtsan.so.0+0x0002914e)
#1 main (a.out+0x0040092e)
SUMMARY: ThreadSanitizer: data race
(/home/ed/gnu/gcc-test/a.out+0x40088e) in t1
==
ThreadSanitizer: reported 1 warnings
The warning goes away when the main function explicitly
joins the thread t1.
That is what I do with all my threads whenever possible,
maybe there is a way how you could explicitly join
all running threads?
Bernd.
Re: [PATCH] x32: Add and use libgnarl/s-taprop__x32.adb
> s-taprop.adb failed to compile for x32: > > s-taprop.adb:341:29: operator for type "System.Linux.time_t" is not > directly visible > s-taprop.adb:341:29: add with_clause and use_clause for "Linux" > > which is caused by > > 2017-09-25 Doug Rupp > > * libgnarl/s-taprop__linux.adb (Base_Monotonic_Clock): New > variable. > (Compute_Base_Monotonic_Clock): New function. > (Timed_Sleep): Adjust to use Base_Monotonic_Clock. > (Timed_Delay): Likewise. > (Monotonic_Clock): Likewise. > * s-oscons-tmplt.c (CLOCK_MONOTONIC): Use on Linux. > > This patch adds and uses libgnarl/s-taprop__x32.adb for x32, which is > similar to libgnarl/s-taprop__linux.adb, but uses > System.Linux.time_t. > > OK for trunk? No, we don't want more almost-duplicates of s-taprop, we need to find a way to merge the changes with s-taprop__linux.adb, so reason this can't be achieved. Arno
Re: isl scheduler and spatial locality (Re: [PATCH][GRAPHITE] More TLC)
On Sat, Sep 30, 2017 at 07:47:43PM +0200, Richard Biener wrote:
> On September 29, 2017 9:58:41 PM GMT+02:00, Sebastian Pop
> wrote:
> >On Fri, Sep 29, 2017 at 2:37 PM, Sven Verdoolaege
> > wrote:
> >> [Sorry for the resend; I used the wrong email address to CC Alex]
> >>
> >> On Wed, Sep 27, 2017 at 02:18:51PM +0200, Richard Biener wrote:
> >>> Ah, so I now see why we do not perform interchange on trivial cases
> >like
> >>>
> >>> double A[1024][1024], B[1024][1024];
> >>>
> >>> void foo(void)
> >>> {
> >>> for (int i = 0; i < 1024; ++i)
> >>> for (int j = 0; j < 1024; ++j)
> >>> A[j][i] = B[j][i];
> >>> }
> >>
> >> I didn't see you mentioning _why_ you expect an interchange here.
> >> Are you prehaps interested in spatial locality?
> >> If so, then there are several approaches for taking
> >> that into account.
> >> - pluto performs an intra-tile loop interchange to
> >> improve temporal and/or spatial locality. It shouldn't
> >> be too hard to do something similar on an isl generated
> >> schedule
> >> - Alex (Oleksandr) has been working on an extension of
> >> the isl scheduler that takes into account spatial locality.
> >> I'm not sure if it's publicly available.
> >> - I've been working on a special case of spatial locality
> >> (consecutivity). The current version is available in
> >> the consecutivity branch. Note that it may get rebased and
> >> it may not necessarily get merged into master.
> >>
> >> There are also other approaches, but they may not be that
> >> easy to combine with the isl scheduler.
> >
> >Would the following work?
> >Add to the proximity relation the array accesses from two
> >successive iterations of the innermost loop:
> >A[j][i] -> A[j][i+1] and B[j][i] -> B[j][i+1]
> >With these two extra relations in the proximity map,
> >isl should be able to interchange the above loop.
>
> Can anyone give a hint on how to do that in ISL terms?
For the approach pluto is taking, you'll have to look at the source
code, see pluto_intra_tile_optimize_band.
For the other two approaches I mentioned above, reports will
be made available within the next couple of weeks.
For the last one, there is a sample implementation in the
consecutivity branch of PPCG.
skimo
Re: [PATCH gfortran v2] PR 61450: ICE in gfc_global_used()
Committed as revision r253328 with the change logs 2017-10-01 Dominique d'Humieres PR fortran/61450 * parse.c (gfc_global_used): Replace the gfc_internal_error with an error. 2017-10-01 Dominique d'Humieres PR fortran/61450 * gfortran.dg/binding_label_tests_28.f90: New test. Thanks for the review, Dominique > Le 1 oct. 2017 à 10:37, Thomas Koenig a écrit : > > Hi Dominique, > > The patch is OK for trunk. > > Thanks! > > Thomas
Re: [Patch, fortran] PR77296 - [F03] Compiler Error with allocatable string and associate
Dear Paul, > The attached patch fixes the PR and most of the remaining, if not all, > problems associated with deferred string length targets in the > associate construct. The patch works as expected. It also fixes pr60458 and its duplicate pr65187. Thanks for working on this issue, Dominique
Re: [Patch, fortran] PR77296 - [F03] Compiler Error with allocatable string and associate
Hi Paul, The attached patch fixes the PR and most of the remaining, if not all, problems associated with deferred string length targets in the associate construct. Bootstraps and regtests on FC23/x86_64 - OK for trunk? Yes. Thanks a lot for working on this! Regards Thomas
Re: [patch, libfortran] Fix thead sanitizer issue with libgfortran
Am 01.10.2017 um 10:59 schrieb Bernd Edlinger:
maybe there is a way how you could explicitly join
all running threads?
Yes, that seems to do the trick. Thanks!
Here is a patch which appears to work. It does hit a snag with static
linking, though, because it calls __gthread_self (), and that causes
a segfault with -static :-(.
The test case in question is static_linking_1.f.
This appears to be a general problem, and has been discussed
before, for example in
https://gcc.gnu.org/ml/gcc-help/2010-05/msg00029.html .
What would be the best way to proceed? Modify the behavior of -static
with gfortran?
Regards
Thomas
2017-10-01 Thomas Koenig
PR fortran/66756
PR fortran/82378
* io/io.h: Add field th to gfc_unit. Add prototypes for
lock_unit and trylock_unit.
* io/unit.c (insert_unit): Do not create lock and lock, move to
(gfc_get_unit): here; lock after insert_unit has succeded.
Use lock_unit and trylock_unit instead of __gthread_mutex_lock
and __gthread_mutex_trylock.
(init_units): Do not unlock unit locks for stdin, stdout and
stderr.
(lock_unit): New function.
(trylock_unit): New function.
(close_units): If a unit still has a lock, wait for the
completion of the corresponding thread.
* io/unix.c (find_file): Use lock_unit and trylock_unit instead
of __gthread_mutex_lock and __gthread_mutex_trylock.
(flush_all_units): Likewise.
2017-10-01 Thomas Koenig
PR fortran/66756
PR fortran/82378
* gfortran.dg/openmp-close.f90: New test.
Index: io/io.h
===
--- io/io.h (Revision 253162)
+++ io/io.h (Arbeitskopie)
@@ -661,6 +661,8 @@ typedef struct gfc_unit
int continued;
__gthread_mutex_t lock;
+ /* ID of the thread currently holding the lock. */
+ __gthread_t th;
/* Number of threads waiting to acquire this unit's lock.
When non-zero, close_unit doesn't only removes the unit
from the UNIT_ROOT tree, but doesn't free it and the
@@ -764,6 +766,12 @@ internal_proto(get_unit);
extern void unlock_unit (gfc_unit *);
internal_proto(unlock_unit);
+extern void lock_unit (gfc_unit *);
+internal_proto(lock_unit);
+
+extern int trylock_unit (gfc_unit *);
+internal_proto (trylock_unit);
+
extern void finish_last_advance_record (gfc_unit *u);
internal_proto (finish_last_advance_record);
Index: io/unit.c
===
--- io/unit.c (Revision 253162)
+++ io/unit.c (Arbeitskopie)
@@ -221,9 +221,9 @@ insert (gfc_unit *new, gfc_unit *t)
return t;
}
+/* insert_unit()-- Create a new node, insert it into the treap. It is assumed
+ that the caller holds unit_lock. */
-/* insert_unit()-- Create a new node, insert it into the treap. */
-
static gfc_unit *
insert_unit (int n)
{
@@ -237,7 +237,6 @@ insert_unit (int n)
#else
__GTHREAD_MUTEX_INIT_FUNCTION (&u->lock);
#endif
- __gthread_mutex_lock (&u->lock);
u->priority = pseudo_random ();
unit_root = insert (u, unit_root);
return u;
@@ -361,9 +360,12 @@ retry:
if (created)
{
- /* Newly created units have their lock held already
- from insert_unit. Just unlock UNIT_LOCK and return. */
__gthread_mutex_unlock (&unit_lock);
+
+ /* Nobody outside this address has seen this unit yet. We could safely
+ keep it unlocked until now. */
+
+ lock_unit (p);
return p;
}
@@ -371,7 +373,7 @@ found:
if (p != NULL && (p->child_dtio == 0))
{
/* Fast path. */
- if (! __gthread_mutex_trylock (&p->lock))
+ if (! trylock_unit (p))
{
/* assert (p->closed == 0); */
__gthread_mutex_unlock (&unit_lock);
@@ -386,7 +388,7 @@ found:
if (p != NULL && (p->child_dtio == 0))
{
- __gthread_mutex_lock (&p->lock);
+ lock_unit (p);
if (p->closed)
{
__gthread_mutex_lock (&unit_lock);
@@ -616,10 +618,9 @@ init_units (void)
u->endfile = NO_ENDFILE;
u->filename = strdup (stdin_name);
+ u->th = __gthread_self ();
fbuf_init (u, 0);
-
- __gthread_mutex_unlock (&u->lock);
}
if (options.stdout_unit >= 0)
@@ -647,10 +648,9 @@ init_units (void)
u->endfile = AT_ENDFILE;
u->filename = strdup (stdout_name);
+ u->th = __gthread_self ();
fbuf_init (u, 0);
-
- __gthread_mutex_unlock (&u->lock);
}
if (options.stderr_unit >= 0)
@@ -677,11 +677,10 @@ init_units (void)
u->endfile = AT_ENDFILE;
u->filename = strdup (stderr_name);
+ u->th = __gthread_self ();
fbuf_init (u, 256); /* 256 bytes should be enough, probably not doing
any kind of exotic formatting to stderr. */
-
- __gthread_mutex_unlock (&u->lock);
}
/* Calculate the maximum file offset in a portable manner.
Re: [Patch, fortran] PR 82312 - [7/8 Regression] Pointer assignment to component of class variable results wrong vptr for the variable
Hi Paul, Bootstraps and regtests on FC23/x86_64 - OK for trunk and 7 branch? OK for both. Thanks for the patch! Regards Thomas
Re: [patch, libfortran] Fix thead sanitizer issue with libgfortran
On 10/01/17 15:41, Thomas Koenig wrote: > Am 01.10.2017 um 10:59 schrieb Bernd Edlinger: >> maybe there is a way how you could explicitly join >> all running threads? > > Yes, that seems to do the trick. Thanks! > Oh, that is really very surprising... I believe that all omp threads are created in detached state, so pthread_join should be undefined on them, just tsan *thinks* otherwise? When I look further on the libgomp sources, I see there are two completely different implementations of the mutexes, barriers, etc. One using posix functions which should be visible to tsan and another one using raw linux futex calls which should be invisible to tsan, by default the linux system calls are used, which explains why tsan seems to be unaware of the actual synchronization in this example. Have you tried to build with --enable-linux-futex=no ? I just saw the following line in libgomp/configure.tgt: # Since we require POSIX threads, assume a POSIX system by default. config_path="posix" # Check for futex enabled all at once. if test x$enable_linux_futex = xyes; then So maybe completely different things will be reported if this value is set to no? > Here is a patch which appears to work. It does hit a snag with static > linking, though, because it calls __gthread_self (), and that causes > a segfault with -static :-(. > > The test case in question is static_linking_1.f. > > This appears to be a general problem, and has been discussed > before, for example in > https://gcc.gnu.org/ml/gcc-help/2010-05/msg00029.html . > > What would be the best way to proceed? Modify the behavior of -static > with gfortran? > > Regards > > Thomas > > 2017-10-01 Thomas Koenig > > > PR fortran/66756 > PR fortran/82378 > * io/io.h: Add field th to gfc_unit. Add prototypes for > lock_unit and trylock_unit. > * io/unit.c (insert_unit): Do not create lock and lock, move to > (gfc_get_unit): here; lock after insert_unit has succeded. > Use lock_unit and trylock_unit instead of __gthread_mutex_lock > and __gthread_mutex_trylock. > (init_units): Do not unlock unit locks for stdin, stdout and > stderr. > (lock_unit): New function. > (trylock_unit): New function. > (close_units): If a unit still has a lock, wait for the > completion of the corresponding thread. > * io/unix.c (find_file): Use lock_unit and trylock_unit instead > of __gthread_mutex_lock and __gthread_mutex_trylock. > (flush_all_units): Likewise. > > 2017-10-01 Thomas Koenig > > PR fortran/66756 > PR fortran/82378 > * gfortran.dg/openmp-close.f90: New test.
[committed][PATCH] Simplify relationals into simple equality conditionals in DOM
A short while ago Martin Liska posted a patch that lowered certain
switch statements into cascading conditionals.
His work tripped two regressions in the testsuite, both cases where we
did not optimize as well as we should have.
Upon investigation I realized a simple improvement to DOM would fix
things up. Further testing showed that the situation occurred
reasonably often in practice and that the situation did occur even when
VRP was enabled.
What we want to do is detect cases where we have something like this in
the expression hash table
TRUE = (i <= 1)
And we're presented with the condition we want to optimize like (i >= 1)
The only value of i that satisfies both is i == 1 So we can change the
conditional to (i == 1).
The simplified conditional is useful because it exposes a constant in
one arm of the conditional which we can propagate. Furthermore the
equality test is easier for tree-ssa-uninit.c to consume.
The implementation is pretty simple. For X GE/LE Y we lookup X LE/GE Y
and if we get a hit then we know we can optimize the conditional to X ==
Y. If Y is a constant, we can handle GT/LT with trivial canonicalization.
The testcase I've added is potentially overly simplistic -- it'll likely
be compromised by work from Aldy or Andrew at some point. We'll likely
have to reduce a new testcase at that time.
Bootstrapped and regression tested on x86_64. Also verified the new
test passes on ppc64le. Installing on the trunk.
Jeff
commit 2cea47f2d18143e46f33eb9a968af0ab1159b1ee
Author: law
Date: Sun Oct 1 15:22:39 2017 +
* tree-ssa-dom.c (optimize_stmt): Make this a method within the
dom_opt_dom_walker class with direct access to private members.
Add comments. Call test_for_singularity.
(dom_opt_dom_walker::before_dom_children): Corresponding changes.
(dom_opt_dom_walker::after_dom_children): Do not lazily initialize
m_dummy_cond anymore.
(class dom_opt_dom_walker): Initialize m_dummy_cond member in the
class ctor.
(pass_dominator:execute): Build the dummy_cond here and pass it
to the dom_opt_dom_walker ctor.
(test_for_singularity): New function.
* gcc.dg/tree-ssa/ssa-dom-simplify-1.c: New test.
2017-09-30 Paolo Carlini
git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@253329
138bc75d-0d04-0410-961f-82ee72b054a4
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index d1c3e9fa409..93dcaedbb4a 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,17 @@
+2017-10-01 Jeff Law
+
+ * tree-ssa-dom.c (optimize_stmt): Make this a method within the
+ dom_opt_dom_walker class with direct access to private members.
+ Add comments. Call test_for_singularity.
+ (dom_opt_dom_walker::before_dom_children): Corresponding changes.
+ (dom_opt_dom_walker::after_dom_children): Do not lazily initialize
+ m_dummy_cond anymore.
+ (class dom_opt_dom_walker): Initialize m_dummy_cond member in the
+ class ctor.
+ (pass_dominator:execute): Build the dummy_cond here and pass it
+ to the dom_opt_dom_walker ctor.
+ (test_for_singularity): New function.
+
2017-09-30 Krister Walfridsson
Maya Rashish
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 40f59343fe1..d12fbdd86a5 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2017-10-01 Jeff Law
+
+ * gcc.dg/tree-ssa/ssa-dom-simplify-1.c: New test.
+
2017-10-01 Dominique d'Humieres
PR fortran/61450
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-simplify-1.c
b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-simplify-1.c
new file mode 100644
index 000..23741b60f65
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dom-simplify-1.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -w -fdump-tree-dom2" } */
+
+extern void frob (void);
+extern void frob (void);
+
+void
+rhs_to_tree (int x, int z)
+{
+ if (x >= 4)
+frob ();
+ if (x >= 3)
+frob ();
+}
+
+/* The second conditional should change into a simple equality test. */
+/* { dg-final { scan-tree-dump-times "if \\(x_\[0-9\]+\\(D\\) == 3\\)" 1
"dom2"} } */
+
diff --git a/gcc/tree-ssa-dom.c b/gcc/tree-ssa-dom.c
index d91766e902e..06be69a530c 100644
--- a/gcc/tree-ssa-dom.c
+++ b/gcc/tree-ssa-dom.c
@@ -103,9 +103,6 @@ struct opt_stats_d
static struct opt_stats_d opt_stats;
/* Local functions. */
-static edge optimize_stmt (basic_block, gimple_stmt_iterator,
- class const_and_copies *,
- class avail_exprs_stack *);
static void record_equality (tree, tree, class const_and_copies *);
static void record_equivalences_from_phis (basic_block);
static void record_equivalences_from_incoming_edge (basic_block,
@@ -572,11 +569,12 @@ class dom_opt_dom_walker : public dom_walker
public:
dom_opt_dom_walker (cdi_
Fix mismatched precisions in tree arithmetic
The tree wi:: decompose routine wasn't asserting that the requested
precision matched the tree's precision. This could make a difference
for unsigned trees that are exactly N HWIs wide and that have the upper
bit set, since we then need an extra zero HWI when extending it to wider
precisions (as for wi::to_widest).
This patch adds the assert and fixes the fallout shown by the testsuite.
Go seems to be unaffected.
Other fixed-precision decompose routines already had an assert.
Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linux-gnu.
Also tested by comparing the testsuite assembly output on at least one
target per CPU directory. OK to install?
Richard
2017-10-01 Richard Sandiford
gcc/
* tree.h (wi::int_traits ::decompose): Assert that the
requested precision matches the type's.
* calls.c (alloc_max_size): Calculate the new candidate size as
a widest_int and use wi::to_widest when comparing it with the
current candidate size.
* gimple-ssa-warn-alloca.c (pass_walloca::execute): Compare with
zero rather than integer_zero_node.
* match.pd: Check for a no-op conversion before using wi::add
rather than after. Use tree_to_uhwi when summing small shift
counts into an unsigned int.
gcc/c-family/
* c-warn.c (warn_tautological_bitwise_comparison): Use wi::to_widest
when combining the original unconverted comparison operands.
gcc/cp/
* constexpr.c (cxx_eval_store_expression): Use wi::to_widest
when comparing the array bounds with an ARRAY_REF index.
gcc/ada/
* gcc-interface/decl.c (annotate_value): Use wi::to_widest when
handling the form (plus/mult (convert @0) @1).
Index: gcc/tree.h
===
--- gcc/tree.h 2017-09-25 13:33:39.989814299 +0100
+++ gcc/tree.h 2017-10-01 17:08:55.827120507 +0100
@@ -5176,6 +5176,7 @@ wi::int_traits ::get_precisi
wi::int_traits ::decompose (HOST_WIDE_INT *,
unsigned int precision, const_tree x)
{
+ gcc_checking_assert (precision == TYPE_PRECISION (TREE_TYPE (x)));
return wi::storage_ref (&TREE_INT_CST_ELT (x, 0), TREE_INT_CST_NUNITS (x),
precision);
}
Index: gcc/calls.c
===
--- gcc/calls.c 2017-09-21 12:13:48.336399601 +0100
+++ gcc/calls.c 2017-10-01 17:08:55.825112782 +0100
@@ -1252,9 +1252,8 @@ alloc_max_size (void)
if (unit)
{
- wide_int w = wi::uhwi (limit, HOST_BITS_PER_WIDE_INT + 64);
- w *= unit;
- if (wi::ltu_p (w, alloc_object_size_limit))
+ widest_int w = wi::mul (limit, unit);
+ if (w < wi::to_widest (alloc_object_size_limit))
alloc_object_size_limit = wide_int_to_tree (ssizetype, w);
}
}
Index: gcc/gimple-ssa-warn-alloca.c
===
--- gcc/gimple-ssa-warn-alloca.c2017-03-28 16:19:28.0 +0100
+++ gcc/gimple-ssa-warn-alloca.c2017-10-01 17:08:55.826116645 +0100
@@ -491,7 +491,7 @@ pass_walloca::execute (function *fun)
is_vla ? G_("argument to variable-length array "
"may be too large")
: G_("argument to % may be too large"))
- && t.limit != integer_zero_node)
+ && t.limit != 0)
{
print_decu (t.limit, buff);
inform (loc, G_("limit is %u bytes, but argument "
@@ -504,7 +504,7 @@ pass_walloca::execute (function *fun)
is_vla ? G_("argument to variable-length array "
"is too large")
: G_("argument to % is too large"))
- && t.limit != integer_zero_node)
+ && t.limit != 0)
{
print_decu (t.limit, buff);
inform (loc, G_("limit is %u bytes, but argument is %s"),
Index: gcc/match.pd
===
--- gcc/match.pd2017-09-25 13:57:11.698158636 +0100
+++ gcc/match.pd2017-10-01 17:08:55.827120507 +0100
@@ -358,8 +358,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
(div (convert? (bit_and @0 INTEGER_CST@1)) INTEGER_CST@2)
(if (integer_pow2p (@2)
&& tree_int_cst_sgn (@2) > 0
- && wi::add (@2, @1) == 0
- && tree_nop_conversion_p (type, TREE_TYPE (@0)))
+ && tree_nop_conversion_p (type, TREE_TYPE (@0))
+ && wi::add (@2, @1) == 0)
(rshift (convert @0) { build_int_cst (integer_type_node,
wi::exact_log2 (@2)); }
@@ -1871,7 +1871,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
&& wi::lt_p (@1, p
Re: Don't query the frontend for unsupported types
Richard Biener writes:
> On Fri, Sep 22, 2017 at 6:42 PM, Richard Sandiford
> wrote:
>> Richard Biener writes:
>>> On Thu, Sep 21, 2017 at 2:56 PM, Richard Sandiford
>>> wrote:
Richard Biener writes:
> On September 20, 2017 2:36:03 PM GMT+02:00, Richard Sandiford
> wrote:
>>When forcing a constant of mode MODE into memory, force_const_mem
>>asks the frontend to provide the type associated with that mode.
>>In principle type_for_mode is allowed to return null, and although
>>one use site correctly handled that, the other didn't.
>>
>>I think there's agreement that it's bogus to use type_for_mode for
>>this kind of thing, since it forces frontends to handle types that
>>don't exist in that language. See e.g. http://gcc.gnu.org/PR46805
>>where the Go frontend was forced to handle vector types even though
>>Go doesn't have vector types.
>>
>>Also, the frontends use code like:
>>
>> else if (VECTOR_MODE_P (mode))
>>{
>> machine_mode inner_mode = GET_MODE_INNER (mode);
>> tree inner_type = c_common_type_for_mode (inner_mode, unsignedp);
>> if (inner_type != NULL_TREE)
>>return build_vector_type_for_mode (inner_type, mode);
>>}
>>
>>and there's no guarantee that every vector mode M used by backend
>>rtl has an associated vector type whose TYPE_MODE is M. I think
>>really the type_for_mode hook should only return trees that _do_ have
>>the requested TYPE_MODE, but PR46805 linked above shows that this is
>>likely to have too many knock-on consequences. It doesn't make sense
>>for force_const_mem to ask about vector modes that aren't valid for
>>vector types, so this patch handles the condition there instead.
>>
>>This is needed for SVE multi-register modes, which are modelled as
>>vector modes but are not usable as vector types.
>>
>>Tested on aarch64-linux-gnu, x86_64-linux-gnu and
>>powerpc64le-linus-gnu.
>>OK to install?
>
> I think we should get rid of the use entirely.
I first read this as not using type_for_mode at all in force_const_mem,
which sounded like a good thing :-)
>>>
>>> That's what I meant ;) A mode doesn't really have a type...
>>>
>>> I tried it overnight on the usual
at-least-one-target-per-CPU set and diffing the before and after
assembly for the testsuite. And it looks like i686 relies on this
to get an alignment of 16 rather than 4 for XFmode constants:
GET_MODE_ALIGNMENT (XFmode) == 32 (as requested by i386-modes.def),
but i386's CONSTANT_ALIGNMENT increases it to 128 for static constants.
>>>
>>> Then the issue is that CONSTANT_ALIGNMENT takes a tree and not a mode...
>>> even worse than type_for_mode is a use of make_tree! Incidentially
>>> ix86_constant_alignment _does_ look at the mode in the end...
>>
>> OK, I guess this means another target hook conversion. The patch
>> below converts CONSTANT_ALIGNMENT with its current interface.
>> The definition:
>>
>> #define CONSTANT_ALIGNMENT(EXP, ALIGN) \
>> (TREE_CODE (EXP) == STRING_CST \
>> && (ALIGN) < BITS_PER_WORD ? BITS_PER_WORD : (ALIGN))
>>
>> was very common, so the patch adds a canned definition for that,
>> called constant_alignment_word_strings. Some ports had a variation
>> that used a port-local FASTEST_ALIGNMENT instead of BITS_PER_WORD;
>> the patch uses constant_alignment_word_strings if FASTEST_ALIGNMENT
>> was always BITS_PER_WORD and a port-local hook function otherwise.
>>
>> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linux-gnu.
>> Also tested by comparing the testsuite assembly output on at least one
>> target per CPU directory. I don't think this comes under Jeff's
>> preapproval due to the constant_alignment_word_strings thing, so:
>> OK to install?
>
> Ok.
Thanks. A bit later than intended, but here's the follow-on to add
the new rtx hook.
Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linux-gnu.
Also tested by comparing the testsuite assembly output on at least one
target per CPU directory. OK to install?
Richard
2017-10-01 Richard Sandiford
gcc/
* target.def (static_rtx_alignment): New hook.
* targhooks.h (default_static_rtx_alignment): Declare.
* targhooks.c (default_static_rtx_alignment): New function.
* doc/tm.texi.in (TARGET_STATIC_RTX_ALIGNMENT): New hook.
* doc/tm.texi: Regenerate.
* varasm.c (force_const_mem): Use targetm.static_rtx_alignment
instead of targetm.constant_alignment. Remove call to
set_mem_attributes.
* config/cris/cris.c (TARGET_STATIC_RTX_ALIGNMENT): Redefine.
(cris_preferred_mininum_alignment): New function, split out from...
(cris_constant_alignment): ...here.
(cris_static_rtx_alignment): New function.
* config/i386/i386.c (ix86_static_rtx_alignment): New function,
split out from..
Re: [patch, libfortran] Fix thead sanitizer issue with libgfortran
Hi Bernd, I believe that all omp threads are created in detached state, so pthread_join should be undefined on them, just tsan*thinks* otherwise? When I look further on the libgomp sources, I see there are two completely different implementations of the mutexes, barriers, etc. One using posix functions which should be visible to tsan and another one using raw linux futex calls which should be invisible to tsan, by default the linux system calls are used, which explains why tsan seems to be unaware of the actual synchronization in this example. Have you tried to build with --enable-linux-futex=no ? I just did that, and it appears that the thread sanitizer no longer detects any issues even without my patch (at least on powerpc64-unknown-linux-gnu, gcc110), which showed the behavior before. So, what is the correct way forward? Modifying apparently correct code in libgfortran isn't the answer, I think. Having false positives when users specify -fopenmp -fsanitize=thread is also not nice - this makes debugging multithreaded applications hard. Would it be possible to build a posix-thread-only version of libgomp to be linked in when -fsanitize=thread is supplied? Jakub, any suggestions? Regards Thomas
Re: [PATCH] Fix fortran/81509
2017-09-29 16:08 GMT+02:00 Steve Kargl : > On Fri, Sep 29, 2017 at 11:15:47AM +0200, Janus Weil wrote: >> Hi Steve, >> >> > As aside effect, the patch removes a questionable GNU Fortran >> > extension that allowed arguments to IAND, IOR, and IEOR to have >> > different kind type parameters. The behavior of this extension >> > was not documented. >> >> I don't really like that part. We were using the nice and convenient >> mechanism of gfc_notify_std here, which allows the developer to choose >> via the -std flag whether to strictly adhere to a chosen Fortran >> standard or to allow GNU extensions etc. You're taking away that >> flexibility and replacing it by an unconditional error. I don't >> actually think that's a good idea. >> >> In general one can argue about whether or not it's a good idea to use >> non-std extensions. But I think gfortran should rather leave the >> choice to its users, whether they want to use 'dirty and covenient' >> code or have very strict checking. We have nice mechanisms for this, >> and I do think we should keep them. >> > > It is undefined behavior. ... with regards to the F08 standard, which forbids it, yes. I guess that is the nature of a "non-standard extension". It can still give a meaningful result, after the smaller kind is implicitly converted to the larger kind. Btw, contrary to your earlier claim, the extension is actually documented: https://gcc.gnu.org/onlinedocs/gfortran/IAND.html > I withdraw the patch. Why? It does fix a rejects-valid bug after all, doesn't it? I completely agree with Paul's earlier review that this is good and necessary. All I'm saying is that I don't think it's a good idea to turn the gfc_notify_std into a gfc_error. If you keep the gfc_notify_std, the patch is ok for trunk from my side. Cheers, Janus
Re: [PATCH] Fix fortran/81509
On Sun, Oct 1, 2017 at 8:42 PM, Janus Weil wrote: > 2017-09-29 16:08 GMT+02:00 Steve Kargl : >> On Fri, Sep 29, 2017 at 11:15:47AM +0200, Janus Weil wrote: >>> Hi Steve, >>> >>> > As aside effect, the patch removes a questionable GNU Fortran >>> > extension that allowed arguments to IAND, IOR, and IEOR to have >>> > different kind type parameters. The behavior of this extension >>> > was not documented. >>> >>> I don't really like that part. We were using the nice and convenient >>> mechanism of gfc_notify_std here, which allows the developer to choose >>> via the -std flag whether to strictly adhere to a chosen Fortran >>> standard or to allow GNU extensions etc. You're taking away that >>> flexibility and replacing it by an unconditional error. I don't >>> actually think that's a good idea. >>> >>> In general one can argue about whether or not it's a good idea to use >>> non-std extensions. But I think gfortran should rather leave the >>> choice to its users, whether they want to use 'dirty and covenient' >>> code or have very strict checking. We have nice mechanisms for this, >>> and I do think we should keep them. >>> >> >> It is undefined behavior. > > ... with regards to the F08 standard, which forbids it, yes. I guess > that is the nature of a "non-standard extension". It can still give a > meaningful result, after the smaller kind is implicitly converted to > the larger kind. > > Btw, contrary to your earlier claim, the extension is actually > documented: https://gcc.gnu.org/onlinedocs/gfortran/IAND.html Yes, it's documented, and Steve's patch would have removed those words so I presume he was aware of that. I think the crux was that yes, the existence of such an extension was documented, but not the semantics. Is the smaller kind variable sign-extended to match the larger kind? Or zero-extended? Or is the larger kind variable truncated (presumably not, due to the wording in the return value section)? Or something else? And are whatever semantics that are actually implemented guaranteed in some way or are they just accidents due to how the compiler internals happen to work without any thought into it? Furthermore, is such an extension useful? I don't see bitwise logical operations on different sized types making much sense, whatever semantics we ostensibly support. >> I withdraw the patch. > > Why? It does fix a rejects-valid bug after all, doesn't it? I > completely agree with Paul's earlier review that this is good and > necessary. > > All I'm saying is that I don't think it's a good idea to turn the > gfc_notify_std into a gfc_error. If you keep the gfc_notify_std, the > patch is ok for trunk from my side. Maybe handling both the standard-conforming BOZ semantics and different kind types would have made the patch significantly more complicated, so Steve opted to just delete the non-specified different-kinds extension? Personally, I'm be Ok with Steve's patch as is. -- Janne Blomqvist
MAINTAINERS - maintainership also covers web pages, docs, testsuite
When talking with David (Malcolm) at the Tools Cauldron, he hinted and/or I realized that documenting the below in our MAINTAINERS file might be a good idea. I went ahead and applied the patch below. (Mike, this is also based on some comments of yours.) Thanks! Gerald 2017-10-01 Gerald Pfeifer * MAINTAINERS: Add a note that maintainership also includes web pages, docs, and testsuite related to that area. Index: MAINTAINERS === --- MAINTAINERS (revision 253329) +++ MAINTAINERS (working copy) @@ -37,6 +37,9 @@ the compiler or associated libraries, they still need approval for their own patches from other maintainers or reviewers. +Also note that maintainership of any area covers changes to web pages, +docs, and the testsuite related to that. + CPU Port Maintainers(CPU alphabetical order) aarch64 port Richard Earnshaw
Re: [PATCH] Fix fortran/81509
On Sun, Oct 01, 2017 at 07:42:09PM +0200, Janus Weil wrote: > 2017-09-29 16:08 GMT+02:00 Steve Kargl : > > On Fri, Sep 29, 2017 at 11:15:47AM +0200, Janus Weil wrote: > >> Hi Steve, > >> > >> > As aside effect, the patch removes a questionable GNU Fortran > >> > extension that allowed arguments to IAND, IOR, and IEOR to have > >> > different kind type parameters. The behavior of this extension > >> > was not documented. > >> > >> I don't really like that part. We were using the nice and convenient > >> mechanism of gfc_notify_std here, which allows the developer to choose > >> via the -std flag whether to strictly adhere to a chosen Fortran > >> standard or to allow GNU extensions etc. You're taking away that > >> flexibility and replacing it by an unconditional error. I don't > >> actually think that's a good idea. > >> > >> In general one can argue about whether or not it's a good idea to use > >> non-std extensions. But I think gfortran should rather leave the > >> choice to its users, whether they want to use 'dirty and covenient' > >> code or have very strict checking. We have nice mechanisms for this, > >> and I do think we should keep them. > >> > > > > It is undefined behavior. > > ... with regards to the F08 standard, which forbids it, yes. I guess > that is the nature of a "non-standard extension". It can still give a > meaningful result, after the smaller kind is implicitly converted to > the larger kind. F95 and F03 have IAND(I,J) Arguments. I shall be of type integer. J shall be of type integer with the same kind type parameter as I. and F08 has I shall be of type integer or a boz-literal-constant. J shall be of type integer or a boz-literal-constant. If both I and J are of type integer, they shall have the same kind type parameter. I and J shall not both be boz-literal-constants. If a user wants to mix the kind type parameters, then the user can use AND, OR, and XOR. > Btw, contrary to your earlier claim, the extension is actually > documented: https://gcc.gnu.org/onlinedocs/gfortran/IAND.html A parenthetical remark hardly counts as documentation. >> I withdraw the patch. > > Why? It does fix a rejects-valid bug after all, doesn't it? I > completely agree with Paul's earlier review that this is good and > necessary. > > All I'm saying is that I don't think it's a good idea to turn the > gfc_notify_std into a gfc_error. If you keep the gfc_notify_std, the > patch is ok for trunk from my side. gfortran should encourage users to write standard conforming code. % cat a.f90 program foo integer(4) :: i = 4 integer(8) :: j = 5 print *, iand(i,j) end program foo % gfortran6 -Wall -c a.f90 % gfortran6 -Wconversion -c a.f90 % gfortran6 -Wall -Wextra -c a.f90 % gfortran6 -Wall -Wextra -Wsurprising -c a.f90 There is no mechanism available to warn the user of nonstandard code. Finally, I have neither the time nor energy to debate the merits of a dubious undocumented bug/feature. I have supplied a patch. An objection has been raised. I withdraw the patch. -- Steve 20170425 https://www.youtube.com/watch?v=VWUpyCsUKR4 20161221 https://www.youtube.com/watch?v=IbCHE-hONow
Re: [PATCH] x32: Add and use libgnarl/s-taprop__x32.adb
On 10/1/17, Arnaud Charlet wrote: >> s-taprop.adb failed to compile for x32: >> >> s-taprop.adb:341:29: operator for type "System.Linux.time_t" is not >> directly visible >> s-taprop.adb:341:29: add with_clause and use_clause for "Linux" >> >> which is caused by >> >> 2017-09-25 Doug Rupp >> >> * libgnarl/s-taprop__linux.adb (Base_Monotonic_Clock): New >> variable. >> (Compute_Base_Monotonic_Clock): New function. >> (Timed_Sleep): Adjust to use Base_Monotonic_Clock. >> (Timed_Delay): Likewise. >> (Monotonic_Clock): Likewise. >> * s-oscons-tmplt.c (CLOCK_MONOTONIC): Use on Linux. >> >> This patch adds and uses libgnarl/s-taprop__x32.adb for x32, which is >> similar to libgnarl/s-taprop__linux.adb, but uses >> System.Linux.time_t. >> >> OK for trunk? > > No, we don't want more almost-duplicates of s-taprop, we need to find a > way to merge the changes with s-taprop__linux.adb, so reason this can't be > achieved. > I have no idea how to do it. I opened: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82384 -- H.J.
Re: [PING][PATCH] Output DIEs for outlined OpenMP functions in correct lexical scope
On Fri, 22 Sep 2017 18:08:39 +0200
Jakub Jelinek wrote:
> On Thu, Sep 21, 2017 at 01:08:01PM -0700, Kevin Buettner wrote:
> > Ping.
>
> Ok, thanks.
Committed.
Kevin
> > On Mon, 7 Aug 2017 17:51:38 -0700
> > Kevin Buettner wrote:
> >
> > > On Wed, 10 May 2017 17:24:27 +0200
> > > Jakub Jelinek wrote:
> > >
> > > > What I don't like is that the patch is inconsistent, it sets
> > > > DECL_CONTEXT
> > > > of the child function for all kinds of outlined functions, but then you
> > > > just
> > > > choose one of the many places and add it into the BLOCK tree. Any
> > > > reason
> > > > why the DECL_CONTEXT change can't be done in a helper function together
> > > > with all the changes you've added into omp-expand.c, and then call it
> > > > from
> > > > expand_omp_parallel (with the child_fn and entry_stmt arguments) so that
> > > > you can call it easily also for other constructs, either now or later
> > > > on?
> > >
> > > I've worked out a way to do the DECL_CONTEXT and the scope change
> > > together. The helper function should be usable for other constructs,
> > > though I have not tested this yet.
> > >
> > > > Also, is there any rationale on appending the FUNCTION_DECL to
> > > > BLOCK_VARS
> > > > instead of prepending it there (which is cheaper)? Does the debugger
> > > > care about relative order of those artificial functions vs. other
> > > > variables in the lexical scope?
> > >
> > > To the best of my knowledge, the debugger doesn't care about the
> > > order. I've changed the code to prepend the FUNCTION_DECL to
> > > BLOCK_VARS instead.
> > >
> > > How does this new version (below) look?
> > >
> > > I've done a "make bootstrap" and "make -k check". No regressions found
> > > for x86_64.
> > >
> > > gcc/ChangeLog:
> > >
> > > * omp-expand.c (adjust_context_scope): New function.
> > > (expand_parallel_call): Call adjust_context_scope.
> > > ---
> > > gcc/omp-expand.c | 38 ++
> > > 1 file changed, 38 insertions(+)
> > >
> > > diff --git a/gcc/omp-expand.c b/gcc/omp-expand.c
> > > index d6755cd..9eb0a89 100644
> > > --- a/gcc/omp-expand.c
> > > +++ b/gcc/omp-expand.c
> > > @@ -498,6 +498,42 @@ parallel_needs_hsa_kernel_p (struct omp_region
> > > *region)
> > >return false;
> > > }
> > >
> > > +/* Change DECL_CONTEXT of CHILD_FNDECL to that of the parent function.
> > > + Add CHILD_FNDECL to decl chain of the supercontext of the block
> > > + ENTRY_BLOCK - this is the block which originally contained the
> > > + code from which CHILD_FNDECL was created.
> > > +
> > > + Together, these actions ensure that the debug info for the outlined
> > > + function will be emitted with the correct lexical scope. */
> > > +
> > > +static void
> > > +adjust_context_and_scope (tree entry_block, tree child_fndecl)
> > > +{
> > > + if (entry_block != NULL_TREE && TREE_CODE (entry_block) == BLOCK)
> > > +{
> > > + tree b = BLOCK_SUPERCONTEXT (entry_block);
> > > +
> > > + if (TREE_CODE (b) == BLOCK)
> > > +{
> > > + tree parent_fndecl;
> > > +
> > > + /* Follow supercontext chain until the parent fndecl
> > > + is found. */
> > > + for (parent_fndecl = BLOCK_SUPERCONTEXT (b);
> > > +TREE_CODE (parent_fndecl) == BLOCK;
> > > +parent_fndecl = BLOCK_SUPERCONTEXT (parent_fndecl))
> > > + ;
> > > +
> > > + gcc_assert (TREE_CODE (parent_fndecl) == FUNCTION_DECL);
> > > +
> > > + DECL_CONTEXT (child_fndecl) = parent_fndecl;
> > > +
> > > + DECL_CHAIN (child_fndecl) = BLOCK_VARS (b);
> > > + BLOCK_VARS (b) = child_fndecl;
> > > + }
> > > +}
> > > +}
> > > +
> > > /* Build the function calls to GOMP_parallel_start etc to actually
> > > generate the parallel operation. REGION is the parallel region
> > > being expanded. BB is the block where to insert the code. WS_ARGS
> > > @@ -667,6 +703,8 @@ expand_parallel_call (struct omp_region *region,
> > > basic_block bb,
> > >tree child_fndecl = gimple_omp_parallel_child_fn (entry_stmt);
> > >t2 = build_fold_addr_expr (child_fndecl);
> > >
> > > + adjust_context_and_scope (gimple_block (entry_stmt), child_fndecl);
> > > +
> > >vec_alloc (args, 4 + vec_safe_length (ws_args));
> > >args->quick_push (t2);
> > >args->quick_push (t1);
>
> Jakub
[PATCH] [graphite] translate reads and writes in a single traversal of memory ops
The patch moves the code that translates reads and writes to isl representation
in a same loop. This is to avoid traversing the scop blocks and arrays with
memory operations 3 times.
* graphite-dependences.c (scop_get_reads): Move code to...
(scop_get_must_writes): Move code to...
(scop_get_may_writes): Move code to...
(scop_get_reads_and_writes): ... here.
(scop_get_dependences): Call scop_get_reads_and_writes.
---
gcc/graphite-dependences.c | 78 +-
1 file changed, 21 insertions(+), 57 deletions(-)
diff --git a/gcc/graphite-dependences.c b/gcc/graphite-dependences.c
index 4ed9d00..2066b2e 100644
--- a/gcc/graphite-dependences.c
+++ b/gcc/graphite-dependences.c
@@ -63,20 +63,21 @@ add_pdr_constraints (poly_dr_p pdr, poly_bb_p pbb)
return constrain_domain (x, isl_set_copy (pbb->domain));
}
-/* Returns all the memory reads in SCOP. */
+/* Returns an isl description of all memory operations in SCOP. The memory
+ reads are returned in READS and writes in MUST_WRITES and MAY_WRITES. */
-static isl_union_map *
-scop_get_reads (scop_p scop)
+static void
+scop_get_reads_and_writes (scop_p scop, isl_union_map *reads,
+ isl_union_map *must_writes,
+ isl_union_map *may_writes)
{
int i, j;
poly_bb_p pbb;
poly_dr_p pdr;
- isl_space *space = isl_set_get_space (scop->param_context);
- isl_union_map *res = isl_union_map_empty (space);
FOR_EACH_VEC_ELT (scop->pbbs, i, pbb)
{
- FOR_EACH_VEC_ELT (PBB_DRS (pbb), j, pdr)
+ FOR_EACH_VEC_ELT (PBB_DRS (pbb), j, pdr) {
if (pdr_read_p (pdr))
{
if (dump_file)
@@ -86,33 +87,14 @@ scop_get_reads (scop_p scop)
}
isl_union_map *um
= isl_union_map_from_map (add_pdr_constraints (pdr, pbb));
- res = isl_union_map_union (res, um);
+ reads = isl_union_map_union (reads, um);
if (dump_file)
{
fprintf (dump_file, "Reads depedence graph: ");
- print_isl_union_map (dump_file, res);
+ print_isl_union_map (dump_file, reads);
}
}
-}
-
- return isl_union_map_coalesce (res);
-}
-
-/* Returns all the memory must writes in SCOP. */
-
-static isl_union_map *
-scop_get_must_writes (scop_p scop)
-{
- int i, j;
- poly_bb_p pbb;
- poly_dr_p pdr;
- isl_space *space = isl_set_get_space (scop->param_context);
- isl_union_map *res = isl_union_map_empty (space);
-
- FOR_EACH_VEC_ELT (scop->pbbs, i, pbb)
-{
- FOR_EACH_VEC_ELT (PBB_DRS (pbb), j, pdr)
- if (pdr_write_p (pdr))
+ else if (pdr_write_p (pdr))
{
if (dump_file)
{
@@ -121,33 +103,14 @@ scop_get_must_writes (scop_p scop)
}
isl_union_map *um
= isl_union_map_from_map (add_pdr_constraints (pdr, pbb));
- res = isl_union_map_union (res, um);
+ must_writes = isl_union_map_union (must_writes, um);
if (dump_file)
{
fprintf (dump_file, "Must writes depedence graph: ");
- print_isl_union_map (dump_file, res);
+ print_isl_union_map (dump_file, must_writes);
}
}
-}
-
- return isl_union_map_coalesce (res);
-}
-
-/* Returns all the memory may writes in SCOP. */
-
-static isl_union_map *
-scop_get_may_writes (scop_p scop)
-{
- int i, j;
- poly_bb_p pbb;
- poly_dr_p pdr;
- isl_space *space = isl_set_get_space (scop->param_context);
- isl_union_map *res = isl_union_map_empty (space);
-
- FOR_EACH_VEC_ELT (scop->pbbs, i, pbb)
-{
- FOR_EACH_VEC_ELT (PBB_DRS (pbb), j, pdr)
- if (pdr_may_write_p (pdr))
+ else if (pdr_may_write_p (pdr))
{
if (dump_file)
{
@@ -156,16 +119,15 @@ scop_get_may_writes (scop_p scop)
}
isl_union_map *um
= isl_union_map_from_map (add_pdr_constraints (pdr, pbb));
- res = isl_union_map_union (res, um);
+ may_writes = isl_union_map_union (may_writes, um);
if (dump_file)
{
fprintf (dump_file, "May writes depedence graph: ");
- print_isl_union_map (dump_file, res);
+ print_isl_union_map (dump_file, may_writes);
}
}
+ }
}
-
- return isl_union_map_coalesce (res);
}
/* Helper function used on each MAP of a isl_union_map. Computes the
@@ -300,9 +262,11 @@ scop_get_dependences (scop_p scop)
if (scop->dependence)
return;
- isl_union_map *reads = scop_get_reads (scop);
- isl_union_map *must_writes = scop_get_must_writes (scop);
- isl_union_map *may_writes = scop_get_may_writes (scop);
+ isl_space *space = isl_set_get_space (scop->param_context);
+ isl_union_map *reads = isl_union_map_empty (isl_space_copy (space));
+ isl_union_map *mus
Re: Make tests less istreambuf_iterator implementation dependent
On 28/09/2017 23:56, Jonathan Wakely wrote:
On 28/09/17 21:59 +0200, François Dumont wrote:
The current istreambuf_iterator implementation capture the current
streambuf state each time it is tested for eof or evaluated. This is
why I considered those tests as fragile.
Yes, and I think that's non-conforming.
Good, then we have something to fix.
As I said in the other thread, I'd really like to see references to
the standard used to justify any changes to our istreambuf_iterator
I think _M_c has been introduced to cover this paragraph of the Standard:
24.6.3.1
"1. Class istreambuf_iterator::proxy is for exposition
only. An implementation is permit-
ted to provide equivalent functionality without providing a class with
this name. Class istreambuf_-
iterator::proxy provides a temporary placeholder as the
return value of the post-
increment operator (operator++). It keeps the character pointed to by
the previous value of the iterator for
some possible future access to get the character."
This is why it is being set in operator++(int):
istreambuf_iterator __old = *this;
__old._M_c = _M_sbuf->sbumpc();
It is also the reason why libstdc++ fails:
http://llvm.org/svn/llvm-project/libcxx/trunk/test/std/iterators/stream.iterators/istreambuf.iterator/istreambuf.iterator.cons/proxy.pass.cpp
It looks like this test is simply not Standard conformant.
I guess at some point _M_c started to be used also to cache the
streambuf::sgetc resulting in current situation.
In attached patch I limit usage of _M_c to cover 24.6.3.1.1 point and so
made additional changes to 2.cc test case to demonstrate it.
François
diff --git a/libstdc++-v3/include/bits/streambuf_iterator.h b/libstdc++-v3/include/bits/streambuf_iterator.h
index f0451b1..94d8c7f 100644
--- a/libstdc++-v3/include/bits/streambuf_iterator.h
+++ b/libstdc++-v3/include/bits/streambuf_iterator.h
@@ -95,7 +95,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// NB: This implementation assumes the "end of stream" value
// is EOF, or -1.
mutable streambuf_type* _M_sbuf;
- mutable int_type _M_c;
+ int_type _M_c;
public:
/// Construct end of input stream iterator.
@@ -103,7 +103,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
: _M_sbuf(0), _M_c(traits_type::eof()) { }
#if __cplusplus >= 201103L
- istreambuf_iterator(const istreambuf_iterator&) noexcept = default;
+ istreambuf_iterator(const istreambuf_iterator&) = default;
~istreambuf_iterator() = default;
#endif
@@ -122,28 +122,29 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
char_type
operator*() const
{
+ int_type __c = _M_get();
+
#ifdef _GLIBCXX_DEBUG_PEDANTIC
// Dereferencing a past-the-end istreambuf_iterator is a
// libstdc++ extension
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(!_S_at_eof(__c),
_M_message(__gnu_debug::__msg_deref_istreambuf)
._M_iterator(*this));
#endif
- return traits_type::to_char_type(_M_get());
+ return traits_type::to_char_type(__c);
}
/// Advance the iterator. Calls streambuf.sbumpc().
istreambuf_iterator&
operator++()
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf &&
+(!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
- if (_M_sbuf)
- {
+
_M_sbuf->sbumpc();
_M_c = traits_type::eof();
- }
return *this;
}
@@ -151,16 +152,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
istreambuf_iterator
operator++(int)
{
- __glibcxx_requires_cond(!_M_at_eof(),
+ __glibcxx_requires_cond(_M_sbuf &&
+(!_S_at_eof(_M_c) || !_S_at_eof(_M_sbuf->sgetc())),
_M_message(__gnu_debug::__msg_inc_istreambuf)
._M_iterator(*this));
istreambuf_iterator __old = *this;
- if (_M_sbuf)
- {
__old._M_c = _M_sbuf->sbumpc();
_M_c = traits_type::eof();
- }
return __old;
}
@@ -176,26 +175,21 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
int_type
_M_get() const
{
- const int_type __eof = traits_type::eof();
- int_type __ret = __eof;
- if (_M_sbuf)
- {
- if (!traits_type::eq_int_type(_M_c, __eof))
- __ret = _M_c;
- else if (!traits_type::eq_int_type((__ret = _M_sbuf->sgetc()),
- __eof))
- _M_c = __ret;
- else
+ int_type __ret = _M_c;
+ if (_M_sbuf && _S_at_eof(__ret) && _S_at_eof(__ret = _M_sbuf->sgetc()))
_M_sbuf = 0;
- }
return __ret;
}
bool
_M_at_eof() const
+ { return _S_at_eof(_M_get()); }
+
+ static bool
+ _S_at_eof(int_type __c)
{
const int_type __eof = traits_type::eof();
- return traits_type::eq_int_type(_M_get(), __eof);
+ return traits_type::eq_int_type(__c, __eof);
}
};
@@ -373,13 +367,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
typedef typename __is_iterator_type::traits_type traits_type;
typedef typename __is_iterator_type::streambuf_type s
