OpenMP segfault in relaxation time solver about phoebe

[anjohan]

I don't want to do the other things I need to do today, so I'll see if I can fix this.

[cepellotti]

Well, I did modify that function yesterday, but I don't understand how it could cause an error. In my partial defense, I am running with 2 OMP threads and 4 MPI processes on my laptop and the silicon example runs fine to completion.

[anjohan]

They may also have been reintroduced by a merge (there was another issue that showed up as well, so it seems we messed something up).

[jcoulter12]

@cepellotti -- was there anything yesterday that might have changed in divideWorkIter when you fixed those merge issues yesterday?

[jcoulter12]

I'm also going to quickly check an older copy of phoebe (pre-recent pull request) to see if this is the result of any of the related changes.

[anjohan]

It still looks correct to me, it just isn't thread safe because divideWorkIter sets some member vectors

[anjohan]

I made this function thread-safe on the omp-fix branch, just changing it to

std::vector<int> MPIcontroller::divideWorkIter(size_t numTasks) {
  // return a vector of the start and stop points for task division
  int start = (numTasks * rank) / size;
  int stop = (numTasks * (rank + 1)) / size;
  std::vector<int> divs;
  for (int i = start; i < stop; i++) {
    divs.push_back(i);
  }
  return divs;
}

It no longer segfaults, but it does give the wrong results with openmp. And I have no idea where to start on that.

I guess any parallel loop that relies on divideWork is problematic.

[cepellotti]

It does give wrong results because other parts rely on the member variables that are set by divideWork() (the members workDivision*).

[jcoulter12]

I thought I removed those workDivision members a while ago -- out of curiosity, why do we want to store those?

[cepellotti]

I thought you developed that part :D they are used for mpi->gather and gatherv

…

On Aug 11 2020, at 3:33 pm, jcoulter12 ***@***.***> wrote: I thought I removed those workDivision members a while ago -- out of curiosity, why do we want to store those? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (#55 (comment)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AF4O45OLFJEUDXYG6GHFJ5DSAGMHDANCNFSM4P3M6WSQ).

[jcoulter12]

No I removed them here: 36cc228 , I think we discussed that they weren't necessary because we could just return start/stop or iterators.

[jcoulter12]

Should I remove them again today? This would fix the wrongness of the result.

[cepellotti]

yes please. It goes back to the merge conflict I mis-solved yesterday.

…

On Aug 11 2020, at 3:35 pm, jcoulter12 ***@***.***> wrote: No I removed them here: 36cc228 (36cc228) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (#55 (comment)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/AF4O45LNGX6OBWHKZSSK72TSAGMRJANCNFSM4P3M6WSQ).

[jcoulter12]

Ok, I'll redo this by the end of today -- glad @anjohan caught this now!

[jcoulter12]

This was an error related to different compiler optimizations. Should be fixed by ba60d09