[Openmp-commits] [PATCH] D74092: Changed omp_get_max_threads() implementation to more closely match spec description.

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

This is a bug fix that is correct and helpful, one minor nit inlined but other than that LGTM.

In D74092#1862799 <https://reviews.llvm.org/D74092#1862799>, @JonChesterfield wrote:

> Change looks good to me. @jdoerfert, @abataev, @grokos?
>
> > Is the plan to convert all tests so that they support different architectures in the future and move them to common?
>
> That sounds reasonable, though I'm not sure it's established as a plan. A fair amount of (to be added) tests should be totally architecture agnostic so those should end up under common. And some will always be architecture dependent.


+1, yes most should be independent and only if we have to we go dependent.

In D74092#1861046 <https://reviews.llvm.org/D74092#1861046>, @estewart08 wrote:

> I can definitely add the change to max_threads.c to this review. The CHECK would become 64 due to the fact we are counting all threads now with this proposed change 32 thread_limit + 32 master warp.
>
>   // CHECK: Non-SPMD MaxThreadsL1 = 64
>
>
> Yes, the test I proposed would be for nvptx only due to the fact that the other tests reside in the nvptx directory and the original max_threads test was checking nvptx values as well. Is the plan to convert all tests so that they support different architectures in the future and move them to common?




1. The entire 32 + "32 master warp" is a problematic construction we should get rid of. It's an implementation detail that leaks out and confuses people.
2. If we have different "warp sizes" we can have multiple check prefixes, especially since we will have to generalize `compile-run-and-check` anyway.



================
Comment at: openmp/libomptarget/deviceRTLs/nvptx/test/api/max_threads.c:22
 
-  // CHECK: Non-SPMD MaxThreadsL1 = 32
+  // CHECK: Non-SPMD MaxThreadsL1 = 64
   printf("Non-SPMD MaxThreadsL1 = %d\n", MaxThreadsL1);
----------------
Nit:  Please add a "Fixme" comment here explaining why 32, or actually "WARP_SIZE" would be the right thing here but why we see 62 instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74092