[Openmp-commits] [PATCH] D64808: [OPENMMP] Resolve lost LoopTripCnt for subsequent loops in same thread.

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added inline comments.


================
Comment at: libomptarget/test/offloading/looptripcnt.c:38
+  printf("num_threads %d num_teams %d\n", num_threads[0], num_teams[0]);
+// DEBUG: loop trip count is 128
+  return 0;
----------------
ronlieb wrote:
> ABataev wrote:
> > ronlieb wrote:
> > > ABataev wrote:
> > > > I think, instead of the debug messages better to check the results of the `printf`s.
> > > I initially thought about that, but I am worried that depending on what architecture you run on it might produce different numbers of teams, if the thread limit was a lot smaller than on a gpu for example.
> > > 
> > > but what I am really trying to test is that we found the loopTripCount to pass into the runtime.  
> > > 
> > > if you strongly prefer that I still do so, I will be happy to change the test. I would of course still keep the DP message for diagnostic purposes down the road.
> > It is always better to check the real results rather than debug messages. If you could write a test, which is able to provide a stable check, then do this.
> ok, so I spent some time trying to do this. The issue I am encountering is 
> that the behavior that needs to be verified is that we properly find the loop trip count in  the function 'target' within omptarget.cpp. This is then passed along to function __tgt_rtl_run_target_team_region in plugins.cpp which  "may" choose to modify the blocksPerGrid or not. So depending on your target, you may see different numbers of teams active if you query omp_get_num_teams().  The observed behavior on different hosts is tenuous at best in determining if loop_trip_count was properly found. The point of this test is to make sure we dont loose the loop_trip_count. In this situation i think it best to catch the problem in Debug build/test where we can explicitly observe the DP message.
> 
Then better to check for the debug message, I think. Other opinions?


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D64808