HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

OpenMP parallelization of a for loop with function calls

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
callswithloopfunctionopenmpparallelizationfor

Problem

Using OpenMP, is it correct to parallelize a for loop inside a function "func" as follows?

void func(REAL coeff, DATAMPOT *dmp, int a, int la, int b, int lb, REAL L)
{
    int i,j,k;
    REAL dx,dy,dz;
    REAL dx2,dy2,dz2;
    REAL r;

    #pragma omp parallel for default(shared) private(k,i,j,dx,dy,dz,dx2,dy2,dz2,r) reduction(+:deltaE)
    for(k=0; knpot>1?
mpot(r,dmp+NSPES*part[i].s+part[j].s,((REAL)rand())/RAND_MAX):
(dmp+NSPES*part[i].s+part[j].s)->pot[0](r,(dmp+NSPES*part[i].s+part[j].s)->dp ) );

    }

}


Where:

  • REAL is double (#define REAL double)



  • DATAMPOT *dmp is a pointer to a struct containing (among others) some pointers to functions, such as pot[0]



  • part is a global array of struct



  • deltaE (variable for summation-reduction) is a REAL global variable



I know that, for a correctness, a special treatment of function rand() is also required;
but apart from that, are there some other important (conceptual) correction to do on the above parallelization? Which is limited at only one directive row?

Solution

-
Try to come up with a better function name than func(). You are the one who understands this code the most, so you should know how to give it a relevant name.

-
Try not to use single-character names, unless they're for a simple for-loop. It's hard to tell what they're for, especially without comments. If this ends up making the #pragma line even longer, then you can just split it into separate lines with a \ at the end of each line.

-
It's a little hard to read lines lacking whitespace:

dx2=(dx<0.5?dx*dx:(1-dx)*(1-dx));


It's difficult to see the entire ternary statement, so add more whitespace:

dx2 = (dx < 0.5 ? dx*dx : (1-dx)*(1-dx));


This concept should be applied everywhere, especially each statement in the loop.

Going back to the ternary, consider using an if/else here instead. Sure, this is a short ternary, but that doesn't mean it should always be used, especially if it's harder to read carefully.

if (dx < 0.5)
{
    dx2 = dx * dx;
}
else
{
    dx2 = (1-dx) * (1-dx);
}


-
This formatting is a bit hard to read:

deltaE += coeff*((dmp+NSPES*part[i].s+part[j].s)->npot>1?
mpot(r,dmp+NSPES*part[i].s+part[j].s,((REAL)rand())/RAND_MAX):
(dmp+NSPES*part[i].s+part[j].s)->pot[0](r,(dmp+NSPES*part[i].s+part[j].s)->dp ) );


At least reformat it to something like this:

deltaE += coeff*((dmp+NSPES*part[i].s+part[j].s)->npot>1
        ? mpot(r,dmp+NSPES*part[i].s+part[j].s,((REAL)rand())/RAND_MAX)
        : (dmp+NSPES*part[i].s+part[j].s)->pot[0](r,(dmp+NSPES*part[i].s+part[j].s)->dp));


However, this may still be too lengthy for a ternary. You could still use a plain if/else if it would help more with readability.

Code Snippets

dx2=(dx<0.5?dx*dx:(1-dx)*(1-dx));
dx2 = (dx < 0.5 ? dx*dx : (1-dx)*(1-dx));
if (dx < 0.5)
{
    dx2 = dx * dx;
}
else
{
    dx2 = (1-dx) * (1-dx);
}
deltaE += coeff*((dmp+NSPES*part[i].s+part[j].s)->npot>1?
mpot(r,dmp+NSPES*part[i].s+part[j].s,((REAL)rand())/RAND_MAX):
(dmp+NSPES*part[i].s+part[j].s)->pot[0](r,(dmp+NSPES*part[i].s+part[j].s)->dp ) );
deltaE += coeff*((dmp+NSPES*part[i].s+part[j].s)->npot>1
        ? mpot(r,dmp+NSPES*part[i].s+part[j].s,((REAL)rand())/RAND_MAX)
        : (dmp+NSPES*part[i].s+part[j].s)->pot[0](r,(dmp+NSPES*part[i].s+part[j].s)->dp));

Context

StackExchange Code Review Q#26515, answer score: 5

Revisions (0)

No revisions yet.