[Bug c++/26943] New: [gomp] firstprivate not working properly with non-POD

[Bug c++/26943] New: [gomp] firstprivate not working properly with non-POD

[rth at gcc dot gnu dot org]

Two problems here.  The first was trying to show that we don't necessarily
honor
the requirement that all firstprivate copies are executed before the
lastprivate
assignment happens.  The second is that we're not properly substituting for the
global X here within the scope of the privatization.


-- 
           Summary: [gomp] firstprivate not working properly with non-POD
           Product: gcc
           Version: 4.2.0
            Status: UNCONFIRMED
          Keywords: wrong-code, openmp
          Severity: normal
          Priority: P3
         Component: c++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: rth at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[rth at gcc dot gnu dot org]

------- Comment #1 from rth at gcc dot gnu dot org  2006-03-30 15:56 -------
Created an attachment (id=11166)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=11166&action=view)
test case


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[jakub at gcc dot gnu dot org]

------- Comment #2 from jakub at gcc dot gnu dot org  2006-04-28 15:07 -------
One incremental fix for the global var case is:
--- omp-low.c.jj5       2006-04-28 13:29:49.000000000 +0200
+++ omp-low.c   2006-04-28 16:22:36.000000000 +0200
@@ -674,9 +674,6 @@ omp_copy_decl (tree var, copy_body_data
   omp_context *ctx = (omp_context *) cb;
   tree new_var;

-  if (is_global_var (var) || decl_function_context (var) != ctx->cb.src_fn)
-    return var;
-
   if (TREE_CODE (var) == LABEL_DECL)
     {
       new_var = create_artificial_label ();
@@ -695,6 +692,9 @@ omp_copy_decl (tree var, copy_body_data
        return new_var;
     }

+  if (is_global_var (var) || decl_function_context (var) != ctx->cb.src_fn)
+    return var;
+
   return error_mark_node;
 }

without this we don't remap privatized global vars except directly in the
omp context that privatized them.
Testcase I was looking at is:
extern void abort (void);
extern void omp_set_dynamic (int);
int n = 6;

int
main (void)
{
  int i, x = 0;
  omp_set_dynamic (0);
#pragma omp parallel for num_threads (16) firstprivate (n) lastprivate (n) \
                         schedule (static, 1) reduction (+: x)
  for (i = 0; i < 16; i++)
    {
      if (n != 6)
        ++x;
      n = i;
    }
  if (x || n != 15)
    abort ();
  return 0;
}

With the above patch, we still create wrong code, with 2 different bugs:
1) there is no barrier to separate firstprivate assignments from lastprivate
2) on the sender side, we store the global var n into
.omp_data_o.4D.1945.nD.1938, but on the receiver side we access either the
remapped private n (assuming the above patch is in) or, when accessing the
outside n we use the global variable n rather than .omp_data_iD.1937.nD.1938

To fix 2), we need to decide where to pass global vars, either the sender side
will do nothing and receiver side will remain as is, or the receiver side will
use .omp_data_i fields even for global vars.  I'd say the sender side should be
changed unless we have some important reason why we need to do an extra copy
(if passed by reference, there is certainly no point in doing that).

As for 1), I believe that whenever there is any
OMP_CLAUSE_LASTPRIVATE_FIRSTPRIVATE clause we need to do the same as we do
for copyin_by_ref, at least for the time being (later on we could replace that
by a more sofisticated special case barrier implementation), because
even if the firstprivate/lastprivate var is a scalar passed by value in
.omp_data_* struct, firstprivate copying copies it from that struct and
lastprivate copying writes it into the same struct same field, so if say
one of threads is delayed for whatever reason, it will execute firstprivate
copying after other thread that handled the last case performed the lastprivate
copying back (well, scalar by value could perhaps be done even without that
by allocating two fields instead of one, one write only and one readonly, but
by reference/is_reference/VLA need to be with barrier in all cases).

In addition to this, I'm not sure what exactly the standard requires say when
firstprivate is used on a global var.  The global var can be visible to the
threads and they can modify it, do we need to make any guarantees about what
exact value gets assigned to the privatized n variable in various threads?
As in, is the following program making a valid assumption or not?
int n = 2;
void bar (void) { n = 4; }
void
foo (void)
{
  if (n != 2)
    return;
  #pragma omp parallel firstprivate (n)
    {
      if (n != 2)
        abort ();
      bar ();
    }
}
}


-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dnovillo at gcc dot gnu dot
                   |                            |org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[dnovillo at gcc dot gnu dot org]

------- Comment #3 from dnovillo at gcc dot gnu dot org  2006-04-29 04:55 -------
(In reply to comment #2)

> In addition to this, I'm not sure what exactly the standard requires say when
> firstprivate is used on a global var.  The global var can be visible to the
> threads and they can modify it, do we need to make any guarantees about what
> exact value gets assigned to the privatized n variable in various threads?
>
No, we do not need to handle this case.  In A.26.2, pg 169:

---------------------------------------------------------------------------------
The private clause of a parallel construct is only in effect inside the
construct,
and not for the rest of the region. Therefore, in the example that follows, any
uses of the
variable a within the loop in the routine f refers to a private copy of a,
while a usage in
routine g refers to the global a.
                                          C/C++
Example A.26.2c
int a;
void g(int k) {
  a = k;             /* The global "a", not the private "a" in f */
}
void f(int n) {
  int a = 0;
  #pragma omp parallel for private(a)
    for (int i=1; i<n; i++) {
         a = i;
         g(a*2);           /* Private copy of "a" */
     }
}
---------------------------------------------------------------------------------

So, we only need to privatize the references inside the construct.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[dnovillo at gcc dot gnu dot org]

-- 

dnovillo at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |dnovillo at gcc dot gnu dot
                   |dot org                     |org
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2006-05-01 12:39:33
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[dnovillo at gcc dot gnu dot org]

------- Comment #4 from dnovillo at gcc dot gnu dot org  2006-05-01 15:15 -------
(In reply to comment #2)

> without this we don't remap privatized global vars except directly in the
> omp context that privatized them.
>
But this is as it should be.  We are only required to privatize variables in
the direct context that privatized them.  We currently aren't, but if 'n' was
accessed inside a separate function, the global 'n' should be accessed.

> With the above patch, we still create wrong code, with 2 different bugs:
> 1) there is no barrier to separate firstprivate assignments from lastprivate
>
Barrier?  We don't need a barrier.  We just need to make sure that only the
thread handling the very last iteration of the loop or the thread executing the
last omp section is the only one executing the copy-out operation.

> 2) on the sender side, we store the global var n into
> .omp_data_o.4D.1945.nD.1938, but on the receiver side we access either the
> remapped private n (assuming the above patch is in) or, when accessing the
> outside n we use the global variable n rather than .omp_data_iD.1937.nD.1938
> 
We should not need to access via .omp_data_o/.omp_data_i.  So, if n.1591 is the
privatized version of n.1567, we should emit something like:

#pragma omp parallel firstprivate(n.1567) lastprivate(n.1567)
  n.1591 = n.1567
  #pragma omp for
  for (i.1587 = 0; i.1587 <= 15; i.1587 = i.1587 + 1)
    < ... use and set n.1591 ...>
  OMP_CONTIUNE
  if (i.1587 == 16)
    n.1567 = n.1591
  OMP_RETURN [nowait]


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[jakub at gcc dot gnu dot org]

------- Comment #5 from jakub at gcc dot gnu dot org  2006-05-01 16:07 -------
We do need a barrier (well, in some cases with extra code we can avoid
it in some cases), in order to honor 2.8.3.4:
"If a list item appears in both firstprivate and lastprivate clauses, the
update
required for lastprivate occurs after all the initializations for
firstprivate."
Now, as e.g. Richard's testcase shows, without a barrier somewhere between
the firstprivate initializations and lastprivate copying it is possible that
some thread is initialized for the first time after the thread handling the
last case executed the lastprivate copying.  That can happen either because
some constructor in firstprivate initialization slept intentionally, or just
the thread was not being scheduled to run for sufficiently long.

The patch I'll post RSN will just add the barrier for any
firstprivate+lastprivate, correctness first, then we can optimize.

Cases which can be optimized:
1) if we prove the structured block has at least one barrier in between the
firstprivate and lastprivate code chunks (doesn't matter if explicit #pragma
omp barrier or some other OMP stuff that acts similarly)
2) if the variable is not passed by reference (i.e. scalar put directly into
.omp_data_*) and we use 2 fields for it rather than one - sender initializes
first field and firstprivate uses that, then lastprivate sets the second field
and sender copies from the second field.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[dnovillo at redhat dot com]

------- Comment #6 from dnovillo at redhat dot com  2006-05-01 16:11 -------
Subject: Re:  [gomp] firstprivate not working properly with
 non-POD

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

jakub at gcc dot gnu dot org wrote:
> ------- Comment #5 from jakub at gcc dot gnu dot org  2006-05-01 16:07 -------
> We do need a barrier (well, in some cases with extra code we can avoid
> it in some cases), in order to honor 2.8.3.4:
> "If a list item appears in both firstprivate and lastprivate clauses, the
> update
> required for lastprivate occurs after all the initializations for
> firstprivate."
>
Ah, yes, of course.  Sorry about that.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.3 (GNU/Linux)

iD8DBQFEVjMVUTa2oAUaiwQRAkViAJ4s5n62EohuFxCUWVQGZ1owtoSTcACfZR7i
NgLf43AMmOQ0xLmnl89ZkYQ=
=cjzg
-----END PGP SIGNATURE-----


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[rth at gcc dot gnu dot org]

------- Comment #7 from rth at gcc dot gnu dot org  2006-05-01 17:58 -------
(In reply to comment #5)
> 1) if we prove the structured block has at least one barrier in between the
> firstprivate and lastprivate code chunks (doesn't matter if explicit #pragma
> omp barrier or some other OMP stuff that acts similarly)
> 2) if the variable is not passed by reference (i.e. scalar put directly into
> .omp_data_*) and we use 2 fields for it rather than one - sender initializes
> first field and firstprivate uses that, then lastprivate sets the second field
> and sender copies from the second field.

Both of these are fairly valuable.

But for when optimization fails, I think adding a specialized construct
for this in libgomp would also be helpful.  Something akin to a semaphore
initialized to -thread_count, such that a wait on the semaphore before the
lastprivate is assured that all the firstprivates are done.  One would expect
that the common case is that they will be done, and the semaphore will take
the unlocked fast-path.

I'll look into adding this.  Since we've already shipped this library in fc5,
do you think it's worthwhile adding it to a new version tag?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[jakub at gcc dot gnu dot org]

------- Comment #8 from jakub at gcc dot gnu dot org  2006-05-02 20:03 -------
Subject: Bug 26943

Author: jakub
Date: Tue May  2 20:03:38 2006
New Revision: 113483

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=113483
Log:
        PR c++/26943
        * omp-low.c (maybe_lookup_decl_in_outer_ctx): New function.
        (build_outer_var_ref): Use maybe_lookup_decl_in_outer_ctx
        to find if var will be a global variable even in the nested context.
        (omp_copy_decl): Only check for global variable at the end, it might
        be overridden in outer contexts.
        (scan_sharing_clauses): For global variables don't create a field.
        (lower_rec_input_clauses): Do nothing for global shared variables.
        Emit a barrier at the end of ILIST if there were any decls in both
        firstprivate and lastprivate clauses.
        (lower_send_clauses): Do nothing for global variables except for
        COPYIN.

        * testsuite/libgomp.c/pr26943-1.c: New test.
        * testsuite/libgomp.c/pr26943-2.c: New test.
        * testsuite/libgomp.c/pr26943-3.c: New test.
        * testsuite/libgomp.c/pr26943-4.c: New test.
        * testsuite/libgomp.c++/pr27337.C: Remove barrier.
        * testsuite/libgomp.c++/pr26943.C: New test.

Added:
    trunk/libgomp/testsuite/libgomp.c++/pr26943.C
    trunk/libgomp/testsuite/libgomp.c/pr26943-1.c
    trunk/libgomp/testsuite/libgomp.c/pr26943-2.c
    trunk/libgomp/testsuite/libgomp.c/pr26943-3.c
    trunk/libgomp/testsuite/libgomp.c/pr26943-4.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/omp-low.c
    trunk/libgomp/ChangeLog
    trunk/libgomp/testsuite/libgomp.c++/pr27337.C


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[jakub at gcc dot gnu dot org]

------- Comment #9 from jakub at gcc dot gnu dot org  2006-05-03 12:23 -------
Reassigning to Richard for the optimizations mentioned in #7.
The code in SVN should be correct, but sometimes suboptimal.


-- 

jakub at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|jakub at gcc dot gnu dot org|rth at gcc dot gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[jakub at gcc dot gnu dot org]

------- Comment #10 from jakub at gcc dot gnu dot org  2006-05-03 12:31 -------
I think using GOMP_1.1 symver instead of GOMP_1.0 would be preferrable.
I think you probably want to keep the state in the user code (probably
inside of .omp_data_* structure) so sender could zero it, then one routine
(called after firstprivate block) would keep atomically incrementing the
counter
and if a flag has been set, futex_wake ()ing and another routine (called before
lastprivate block) would set the flag, do an atomic barrier and then in a loop
keep comparing the counter with number of threads and if smaller, futex_wait
on the counter.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[bergner at vnet dot ibm dot com]

------- Comment #11 from bergner at vnet dot ibm dot com  2006-12-05 02:06 -------
The testcase pr26943-2.c fails intermittently for me using unpatched mainline. 
Is anyone else seeing that?  I'm building on powerpc64-linux with -m32. If I
run it a few times, it mainly passes, but every once in a while, I get the
error below:

(gdb) run
Starting program:
/home/bergner/gcc/indexed_load_store/build/gcc-mainline-rtlanal/powerpc64-linux/libgomp/testsuite/pr26943-2.exe 
Breakpoint 1 at 0x100111b4
[Thread debugging using libthread_db enabled]
[New Thread 1074060096 (LWP 29281)]
Breakpoint 1 at 0xfe04344
[New Thread 1082451136 (LWP 29282)]
[New Thread 1090839744 (LWP 29283)]
[New Thread 1099228352 (LWP 29284)]
[Switching to Thread 1074060096 (LWP 29281)]

Breakpoint 1, 0x0fe04344 in abort () from /lib/tls/libc.so.6
(gdb) where
#0  0x0fe04344 in abort () from /lib/tls/libc.so.6
#1  0x100006b8 in main ()
    at
/home/bergner/gcc/indexed_load_store/gcc-mainline-rtlanal/libgomp/testsuite/libgomp.c/pr26943-2.c:45
(gdb) list 42
37            if (e[0] != 'a' + 6 || f[0] != 'b' + i || g[0] != 'g' + i)
38              j += 64;
39            if (h[0] != 'h' + i)
40              j += 512;
41          }
42        if (j || a != 8 + 6 || b != 12 || c != 3 || d != 20)
43          abort ();
44        if (e[0] != 'a' + 6 || f[0] != 'b' || g[0] != 'g' + 3 || h[0] != 'd')
45          abort ();
46        return 0;
(gdb) p e[0]
$10 = 101 'e'
(gdb) p f[0]
$11 = 98 'b'
(gdb) p g[0]
$12 = 106 'j'
(gdb) p h[0]
$13 = 100 'd'


-- 

bergner at vnet dot ibm dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bergner at vnet dot ibm dot
                   |                            |com


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate not working properly with non-POD

[pinskia at gcc dot gnu dot org]

------- Comment #12 from pinskia at gcc dot gnu dot org  2006-12-11 00:36 -------
PR 27557 looks like a dup of this one.


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
OtherBugsDependingO|                            |27557
              nThis|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate + lastprivate uses inefficient barrier

[rth at gcc dot gnu dot org]

------- Comment #13 from rth at gcc dot gnu dot org  2007-03-21 15:43 -------
We've fixed the wrong-code part.
Re-tagging the PR to reflect the optimization possibility.


-- 

rth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement
           Keywords|wrong-code                  |missed-optimization
            Summary|[gomp] firstprivate not     |[gomp] firstprivate +
                   |working properly with non-  |lastprivate uses inefficient
                   |POD                         |barrier


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943

[Bug c++/26943] [gomp] firstprivate + lastprivate uses inefficient barrier

[rth at gcc dot gnu dot org]

-- 

rth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|rth at gcc dot gnu dot org  |unassigned at gcc dot gnu
                   |                            |dot org
             Status|ASSIGNED                    |NEW


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=26943