3

I've recently reading up about OpenMP and was trying to parallelize some existing for loops in my program to get a speed-up. However, for some reason I seem to be getting garbage data written to the file. What I mean by that is I don't have Points 1,2,3,4 etc. written to my file, I have Points 1,4,7,8 etc. I suspect this is because I am not keeping track of the threads and it just leads to race conditions?

I have been reading as much as I can find about OpenMP, since it seems like a great abstraction to do multi-threaded programing. I'd appreciate any pointers please to get to the bottom of what I might be doing incorrectly.

Here is what I have been trying to do so far (only the relevant bit of code):

#include <omp.h>

pixelIncrement = Image.rowinc/2;

#pragma omp parallel for
for (int i = 0; i < Image.nrows; i++ )
   {
      int k =0;
      row     = Image.data + i * pixelIncrement;

      #pragma omp parallel for
      for (int j = 0; j < Image.ncols; j++)
      {
          k++; 
          disparityThresholdValue = row[j];  
                 // Don't want to save certain points
             if ( disparityThresholdValue < threshHold)
             {       
                // Get the data points
                x = (int)Image.x[k]; 
                y = (int)Image.y[k]; 
                z = (int)Image.z[k]; 
                grayValue= (int)Image.gray[k];      

                cloudObject->points[k].x = x;
                cloudObject->points[k].y = y;
                cloudObject->points[k].z = z;
                cloudObject->points[k].grayValue = grayValue;

        fprintf( cloudPointsFile, "%f %f %f %d\n", x, y, z, grayValue);
             }
      }
   }

   fclose( pointFile );

I did enable OpenMP in my Compiler settings (C/C++ -> Language -> Open MP Support (/openmp).

Any suggestions as to what might be the problem? I am using a Quadcore processor on Windows XP 32-bit.

4
  • Seems like you'd want to put 'OpenMP' in the title.
    – Larsenal
    Aug 20, 2012 at 21:59
  • Put #omp atomic just above the fprintf line. You will still not get the data in the correct order, but at least everything should be there in the file. Aug 20, 2012 at 22:02
  • Hristo, can you please elaborate by what do you mean "at least everything should be there in the file." ?
    – c0d3rz
    Aug 20, 2012 at 22:43
  • 1
    I mean that fprintf is not guaranteed to be thread-safe, but in your case the problem is that all variables, defined outside the parallel region, are shared by default and you are experiencing troubles with race conditions. Aug 21, 2012 at 7:55

3 Answers 3

3

Are all points written to the file, but just not sequentially, or is the actual point data messed up?

The first case is expected in parallel programming - once you execute something side-by-side you wont be able to guarantee order unless you synchronize the access (at which point you can just leave out the parallelization as it becomes effectively linear). If you need to rely on order, you can parallelize any calculations but need to write it down in one thread.

If the points itself are messed up, check where your variables are declared and if multiple threads are accessing the same.

2
  • Femaref: the actual point data is messed up. I understand what you mean by the loss of performance when synchronizing. The #omp atomic tag does this right? Can you elaborate on how I can restrict the threads from not accessing the same variables? I've edited my post to reflect a change I had to make to allow it to compile, I think this is because of how I am definining my int k? I can't seem to successfully define 2 ints within the for loop for some reason. I really want to do for (i = 0, k = 0; i < Image.nrows; i++ ), but I get a compile error saying it doesn't know what i is ..
    – c0d3rz
    Aug 20, 2012 at 23:04
  • If I instantiate the variables outside the for loop, the error I get is: "OpenMP 'for' statement has improper form"
    – c0d3rz
    Aug 20, 2012 at 23:16
3

A few problems here:

#pragma omp parallel for
for (int i = 0; i < Image.nrows; i++ )
   {
      int k =0;
      row     = Image.data + i * pixelIncrement;

      #pragma omp parallel for
      for (int j = 0; j < Image.ncols; j++)
      {
          k++; 

There's no need for the inner parallel for. The outer loop should contain enough work to keep all cores busy.

Also, for the inner loop k is a shared variable and gets incremented in a non-atomic way. x, y, z are also shared among the inner loop threads and overwritten "randomly". Remove the inner directive and see how it goes.

2
  • Agreed - trying to parallelize the inner loop as well as the outer one is probably causing problems. Also, you will want to explicitly state which variables are to be private variables such as j, k, row, x, y, z, grayValue and which are shared - threshold, Image, cloudObject, cloudPointsFile. See examples at computing.llnl.gov/tutorials/openMP/#Combined
    – Brian L
    Aug 21, 2012 at 6:43
  • Thanks Tudor, but I still get corrupted entries. I think as Brian suggested, I need to take a closer look and control all the variables being passed ...
    – c0d3rz
    Aug 21, 2012 at 23:32
0

When you have a loop with a nested loop there is no need for a second omp pragma. It will already paralelize the first loop. Remember that this is valid only if the second loop has to be executed in sequence. You have a sequencial incrementation, so you can not execute the second loop in a random order. OMP pragmas are a very easy and cool way to paralelize code but do not use them too much!

More details here -> Parallel Loops with OpenMP

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.