OpenMP parallel for critical section and use of flush

Question

I am not sure about the place where flush should be used (if it is used at all here).

int Window_Shift :: get_window_shift()
{
    int window_shift = INFINITE;    

    #pragma omp parallel for
    for(unsigned int node_id = 0;node_id< no_of_valid_nodes ;node_id++) 
    {
        //for all the nodes that can be extended
        //finds the minimum possible window shift
        if(can_node_determine_window_shift(node_id) == true)
        {
            int node_window_shift = get_window_shift_for_node(node_id);
            #pragma omp flush(window_shift)
            #pragma omp critical(get_window_shift_for_node)
            {
            window_shift = min(window_shift,node_window_shift);
            #pragma omp flush(window_shift)
            }
        }
    }

Please review the code, especially the flush() commands. Should atomic be used instead of a critical section?

Answer 1

• With just one function provided and some return information, it's hard to tell everything about it. Since this is a member function with "get" in its name (meaning it's an accessor), and only local variables are being modified (no data members), then this should likely be a const function.

  int Window_Shift :: get_window_shift() const {}
  

  If you are actually modifying data members somewhere, then disregard this.

• The whitespace in the for loop statement is a little inconsistent:

  for(unsigned int node_id = 0;node_id< no_of_valid_nodes ;node_id++)
  

  This is how it could look:

  for (unsigned int node_id = 0; node_id < no_of_valid_nodes; node_id++)
  

• In a conditional statement, you don't need to explicitly use true or false. It's also safer to avoid this as it'll prevent a possible mismatch of = and ==. This will cause bugs if the compiler doesn't warn you about it (although you should have compiler warnings turned up high).

  This is the same as == true:

  if (someConditional)
  

  This is the same as == false:

  if (!someConditional)
  

• INFINITE isn't defined anywhere, so either this is your own definition, or this is supposed to be INFINITY from <cmath>. You could instead use C++'s alternatives from <limits>.

  If you must use int, which is finite, you can use std::numeric_limits::max() to get its maximum value, which is normally 2³¹ -1:

  int max = std::numeric_limits<int>::max();
  

  If you can use a floating-point type (float or double), then you can get true infinity by using std::numeric_limits::infinity():

  float infinity = std::numeric_limits<float>::infinity();
  

  double infinity = std::numeric_limits<double>::infinity();
  

  More information regarding infinity here.

• You could consider defining private() and shared() for the variables, which goes inside of the parallel for directive. You'll also need default(none) so that the compiler will allow you to set this yourself. This can give you more control over your code, as the compiler would otherwise have to determine what should be private and what should be shared.

• Should atomic be used instead of a critical section?

  You may have to test this yourself, but atomic may be faster. Critical sections, on the other hand, may lead to more serialization, especially if you have one in a loop. Always try to avoid unnecessary serialization in parallel code. Atomics can also cause this, but usually not as greatly since it doesn't lock an entire code section for all of the threads, unlike a critical section.