search

parallel-runtimes / lomp

Little OpenMP Library
Apache License 2.0
153 stars 17 forks source link

Bugfix: libnuma was failing on RISC-V #18

Closed mjklemm closed 3 years ago

mjklemm commented 3 years ago

On RISC-V libnuma is available, but does not seem to be fully functional. Thus, some of the calls reported undefined values and produced an error when the runtime tried to detect the NUMA structure of the RISC-V system. Thus bugfix changes the detection code such that it first determines if libnuma claims to work and if not falls back to a single NUMA domain (like with systems w/o libnuma).

mjklemm commented 3 years ago

@JimCownie Can you please review this one?

JimCownie commented 3 years ago

Wouldn't it simplify the code a lot to test whether libnuma is absent, or doesn't work once at the top, and have an if there that handles that case and returns, then drop down (or have an else if you don't return) that can do the numa stuff without the continual testing of available? I don't see how to checkout your branch (It doesn't show up) I'll mail you some code.

mjklemm commented 3 years ago

Is the extent of auto CoresInDomain = std::vector<int>(); OK? We push it onto the tail of something more persistent, but it is a local variable. Should it be allocated, rather than on the stack?

Yup, that's fine, even though it may not be the most efficient thing to do. The whole std::vector is copied when it is appended. We could change the definition of CoresInDomain to be a vector that is accessed via a smart pointer. Since this code is running only once and then constitutes a read-only database-like structure, I'm not sure if it's worth the effort, though.

JimCownie commented 3 years ago

Looks good to me now.