[ET Trac] [Einstein Toolkit] #928: openmp parallelization within Exact broken

[Einstein Toolkit]

#928: openmp parallelization within Exact broken
------------------------------------+---------------------------------------
  Reporter:  knarf                  |       Owner:                     
      Type:  defect                 |      Status:  confirmed          
  Priority:  minor                  |   Milestone:  ET_2012_11         
 Component:  EinsteinToolkit thorn  |     Version:  development version
Resolution:                         |    Keywords:                     
------------------------------------+---------------------------------------

Comment (by rhaas):

 The attached patch replaces the SAVE variable with THREADPRIVATE variables
 (ie. the per-thread equivalent). I also did this substitution in boost.F77
 where currently a CRITICAL section is used. I believe that the way the the
 critical section was used does not properly address the race condition
 (and I think I sprinkled a couple such constructs through the Cactus
 code). The code and reasoning go like this:

 {{{
 logical,save :: firstcall = .true.
 integer,save :: somevalue

 if (firstcall) then
 $omp critical
 if (firstcall) then
   somevalue = 42
   firstcall = .false.
 end if
 $end critical
 end if
 }}}

 with the aim in skipping the whole critical section once somevalue has
 been initialized and firstcall set to true on the first go. The two if
 statements on the same condition are seens as a first speculative one the
 allows a quick skip and only if we think we might have to do something (ie
 if firstcall.eqv..true.) do we enter the expensive section of code. The
 critical section is intended to ensure we really only do so once.

 The mistake now is that as far as I know, an optimizing compiler is free
 to actually execute the following code:
 {{{
 logical,save :: firstcall = .true.
 integer,save :: somevalue

 if (firstcall) then
 $omp critical
 if (firstcall) then
   ! order of statements reversed
   firstcall = .false.
   somevalue = 42
 end if
 $end critical
 end if
 }}}

 since there is no (visible to the compiler) ordering required. This can go
 wrong in the following way
 1. thread 0 sets firstcall=.false. and flushes this value to memory.
 1. before thread 0 sets somevalue, thread 1 comes along, encounters the
 outer if statement, finds that firstcall=.false.
 1. thread 1 proceeds with the wrong value of somevalue.

 There seems to be no way to force a compiler (C or Fortran) to *not*
 change a value temporarily (or to explicitly enforce an ordering on the
 statements). C's volatile attribute (think of using it to access a
 hardware register) seems the closest (and C also defines synchronization
 points where the compiler must ensure that all variable data is in memory,
 eg. before function calls, though optimization might do funny things as
 well if the compiler thinks it can be sure that a variable is not visible
 to anyone else). None of this helps with Fortran code of course.

-- 
Ticket URL: <https://trac.einsteintoolkit.org/ticket/928#comment:7>
Einstein Toolkit <http://einsteintoolkit.org>
The Einstein Toolkit