This is the mail archive of the fortran@gcc.gnu.org mailing list for the GNU Fortran project.


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

Re: [PATCH/ fortran] PR35423 omp workshare - first patch


Hi,

On Wed, Jul 30, 2008 at 12:15 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> On Sat, Jun 28, 2008 at 12:52:34PM -0500, Vasilis Liaskovitis wrote:
>> attached is a first patch implementing parallelization for some
>> statements within OMP WORKSHARE constructs.
>
> Sorry for the delay, your mail was accidentally caught by SPAM filtering,
> so I only discovered it much later when looking why some mails got lost.
>
> First of all, do you have a Copyright assignment on file?  That's the
> precondition of getting the code accepted.

thanks for reviewing. I don't have a copyright assignment - I sent a
request for forms to assign@gnu.org just now.

> The patch generally looks good, but there are a couple of issues:
>
> 1) I don't see why ws_data.prev_singleunit can't be a local variable in
>   gfc_trans_omp_workshare, no need to make it a field in the global var

you are obviously right, fixed.

> 2) ws_data.workshare_clauses is problematic; the gfc_trans_omp_workshare
>   argument to gfc_trans_omp_workshare will only contain nowait or nothing,
>   so you should just pass that info around as a bool in ws_data, and
>   let both OMP_FOR and OMP_SINGLE create their own clauses tree chain
>   - it shouldn't be shared anyway and for OMP_SINGLE most of the
>   OMP_FOR-ish clauses shouldn't appear.  BTW, no need to add collapse(1)
>   clause, that's the default if no collapse clause is present.

I agree that the nowait boolean value is the only information needed
to be passed around in ws_data. However, since I create OMP_FOR
clauses in different files (e.g. gfc_scalarized_loop_end in
trans-array.c), I 'd need more info to construct the actual tree
clauses there (for example, arguments to gfc_trans_omp_clauses are not
present in the aforementioned function).

The attached patch creates two tree clauses in trans-openmp.c, one for
OMP_FOR clauses and one for OMP_SINGLE clauses i.e. single and for
clauses now do not share clauses. ws_data only passes around the
OMP_FOR clause, since SINGLE clauses are always created locally in
gfc_trans_omp_workshare . The tree chains of workshare FOR and SINGLE
clauses remain the same throughout the handling of the workshare
construct (with dependence analysis we 'd need to pick between a
nowait and wait flavour for both types of clauses as we walk the
gfc_codes). If you still think that more flexibility building tree
chains is going to be needed, let me know. it may require some other
changes.

collapse(1) is no longer added to ompfor clauses. chunk_size and
ordered are also removed, since the clauses argument is guaranteed to
not contain them.

> 3) ws_data as a global variable is problematic with nesting.  Say
> !$omp workshare
>  a(:) = b(:)
>  !$omp parallel
>  c(:) = d(:)
>  !$omp workshare
>  e(:) = f(:)
>  !$omp end workshare
>  !$omp end parallel
>  g(:) = h(:)
> !$omp end workshare
>   This could be solved e.g. by copying ws_data into a temporary variable,
>   clearing it and restoring afterwards around gfc_trans_omp_directive
>   for EXEC_OMP_PARALLEL*.

I am not 100% clear on what the scope of WORKSHARE should be with
regards to a nested PARALLEL region. In my understanding of omp spec
3.0 (section 2.5.4), given the following code

!$OMP PARALLEL
!$OMP WORKSHARE
!$OMP PARALLEL
            SHR = SHR + 1
            AA(:) = BB(:)
!$OMP END PARALLEL
!$OMP END WORKSHARE
!$OMP END PARALLEL

- the inner PARALLEL region should be treated as a single unit of work
by the thread team bound to the WORKSHARE construct (this is the outer
parallel team).
- Workshare semantics do not apply to the statements of the inner
parallel region. So,  the scalar&array assignment are executed by all
threads of the inner parallel team and the array assignment should not
be workshared.

If these are correct assumptions, the new patch should be ok: as you
proposed, the ws_data structure is copied out, reset to 0, and
restored around gfc_trans_omp_directive for EXEC_OMP_PARALLEL*. (these
are done with memset, memcpy though the data structure is rather
small. If desired I can change it to per-field reset/copy)

> 4) EXEC_OMP_BARRIER must not appear in !$omp workshare, so you shouldn't
>   handle it there

done.

> 5) /* barriers and critical regions are executed by each thread */
>   ws_data.curr_singleunit = false;
>   for EXEC_OMP_CRITICAL is wrong, the critical region is a single unit
>   of work and as such executed just by one of the threads.  But
>   you want to clear or save/clear/restore some ws_data fields
>   around gfc_trans_omp_directive for EXEC_OMP_CRITICAL too
>   - the whole critical is single unit of work, so OMP_FOR shouldn't
>   be created in it.

Similarly, ws_data is now copied, reset and restored around
gfc_trans_omp_directive for OMP_CRITICAL. This ensures that e.g. an
OMP_FOR is not created for an array assignment inside OMP_CRITICAL .

We can still get OMP_FOR for a parallel workshared team within OMP_CRITICAL :

!$OMP PARALLEL
!$OMP WORKSHARE
!$OMP CRITICAL
!$OMP PARALLEL
!$OMP WORKSHARE
            DD(:) = BB(:)
!$OMP END WORKSHARE
!$OMP END PARALLEL
!$OMP END CRITICAL
!$OMP END WORKSHARE
!$OMP END PARALLEL

but I think this is the intended behaviour here.

If the above sounds right, the handling of OMP_PARALLEL* and
OMP_CRITICAL is identical. I have merged their handling in the patch.

> 6) you only handle EXEC_OMP_PARALLEL in gfc_trans_omp_workshare,
>   but you should handle EXEC_OMP_PARALLEL_{DO,SECTIONS,WORKSHARE}
>   the same way

done.

> 7) please watch formatting
>   - space between if and (
>   - < 80 column lines
>   - {} for if are idented 2 more spaces from if, the body another
>     2
>   - comments should be sentences, starting with capital letter
>     and ending with ., followed by 2 spaces, not just one
>   - single statement then or else blocks for if don't need {}s around it

sorry for these,  fixed most but there may still be more

> 8) now that trunk has been tuplified, you'll need to change slightly
>   the creation of OMP_FOR (GIMPLE_MODIFY_STMT is gone)

done, now uses MODIFY_EXPR


thanks,

- Vasilis

Attachment: pr35423-b.diff
Description: Binary data


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