[Openmp-commits] [PATCH] D147511: [OpenMP] Fix nextgen plugin behavior when passing negative thread_limit clause

Wed Apr 19 11:06:06 PDT 2023

mhalk added a comment.

In D147511#4280795 <https://reviews.llvm.org/D147511#4280795>, @tianshilei1992 wrote:

> I didn't see that is the case. https://godbolt.org/z/TdcfxfEej I didn't set the thread limit and then it is 0.
> The spec says `thread_limit` has to be greater than zero. So here using 0 to indicate the clause is not set looks good to me.

Sorry, there was some initial confusion: after some investigation it turned out that the negative `thread_limit` came from "outside", which is also why the phrasing of the issue has changed so much.
But as far as I was informed by the reporter of this issue, the result of `getNumThreads` was expected to be `PreferredNumThreads` -- instead, when deliberately setting the `thread_limit` to a negative value, the result is `MaxNumThreads` (as illustrated in the test).

In D147511#4280754 <https://reviews.llvm.org/D147511#4280754>, @jdoerfert wrote:

> Apologies for the delay.
>
> So, this is good and bad.
>
> The bas part is that it actually doesn't matter if the value is "negative". The standard, at least as far as I can remember, doesn't specify if the value is supposed to be 32 bit signed or something else. Hence, `UINT_MAX` as thread limit should just be `UINT_MAX` and not `INT_MIN` which we handle differently than `UINT_MAX/2`.
>
> The good part is that we can actually lower this upper bound, at least if it was not forced (which 5.2 or newer allow, IIRC). Thus, we can lower an unrealistic thread limit to something more meaningful if we want to. However, I don't see why we should make the cutoff at "signed bit set" and not earlier.
>
> Overall, I'd like more reasoning for this threshold so we can potentially come up with a more meaningful way of determining the number of threads, e.g., by looking at the resources used.

No worries!
I like the idea of further lowering the threshold to a "more meaningful" value or determine the return value based on some form of evidence.
I'm primarily curious: //"looking at the resources used"// -- if you find the time, could you elaborate on that?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147511/new/

https://reviews.llvm.org/D147511