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

Wed Apr 19 16:48:27 PDT 2023

jdoerfert requested changes to this revision.
jdoerfert added inline comments.
This revision now requires changes to proceed.

================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:308
                                      ? ThreadLimitClause[0]
                                      : PreferredNumThreads);
 }
----------------
gregrodgers wrote:
> mhalk wrote:
> > jdoerfert wrote:
> > > I don't understand from the commit message what "the problem" was.
> > > This should be min(MNT, TLC[0] or PNT), no? This should never be negative, even if we interpret it signed later. Is the problem that we pick MNT and not PNT?
> > > 
> > > Nit: We should check for <s0 with a cast and comparison, I think that makes the intend clearer.
> > Thank you for the feedback -- didn't want to obscure the problem or intent; I'll fix these.
> > 
> > > Is the problem that we pick MNT and not PNT?
> > Yes, AFAIK we expected to fallback / pick PNT in such a case.
> > Was that a wrong assumption?
> > 
> Yes, spec says it is wrong but spec also says field is int which could have a variable that went incorrectly negative by clumsy user.    Suppose it is -1, the current code adds 64 (warpsize) to the uint and get 63.  YUCK!   Since a negative value is wrong we prefer to treat a negative value at this low in the runtime  as unspecified (clause not present)  which is 0.   To be honest, we are hitting this problem with LLVM IR generated by the legacy fortran which inserted a -1 when there is no thread_limit clause.   Yeah we should fix legacy fortran and ensure LLVM flang treats the lack of clause similarly.  But we were able to reproduce the fail with a variable in the thread_limit clause that is made to go negative by a clumsy user. 
But you don't treat it as 0. That's my point. This patch will use less threads for thread limit 2^31-1 spmd than 2^31 spmd. That doesn't make sense, IMHO.

Let's fix the issue properly, if the additional 64 causes an overflow, don't add them, we will cap the value anyway. 

You still want to fix legacy flang if you want preferred number not max number. That said, i don't see why we need a workaround for legacy flang here.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147511