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

Wed Apr 19 13:24:43 PDT 2023

gregrodgers accepted this revision.
gregrodgers added inline comments.
This revision is now accepted and ready to land.

================
Comment at: openmp/libomptarget/plugins-nextgen/common/PluginInterface/PluginInterface.cpp:308
                                      ? ThreadLimitClause[0]
                                      : PreferredNumThreads);
 }
----------------
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. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147511