[PATCH] D81114: [OpenMPOpt] initial tests for ICV tracking. Only nthreads is used.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/test/Transforms/OpenMP/icv_tracking.ll:61
+  tail call void @omp_set_num_threads(i32 10) #1
+  %5 = tail call i32 @omp_get_max_threads() #1
+  tail call void @use(i32 %5) #1
----------------
sstefan1 wrote:
> jdoerfert wrote:
> > I guess `%5` should be replaced with `min(%3, 10)`, right? We might want to use SCEV to represent the values and combine them.
> Not sure I understand why `min(%3, 10)`? Without any transformations `%5` is 10. Maybe I'm missing something?
> 
> Will look into using SCEV.
In reality, it is a value smaller or equal to 10, I think.

Let's say we have a quad core and the user sets the num_threads to 10, there are two reasonable outcomes. The runtime will cap it at 4, given that any more threads will most likely not help, or it will go with 10. Afaik, OpenMP doesn't guarantee 10 but not more than 10 if you set a limit. We might want to provide user hooks that simplify the logic, e.g., allow us to take 10, but otherwise min(10, %3) seems reasonable to me. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81114