[PATCH] D133949: Make sure the right parameter extension attributes are added in various instrumentation passes.

Mon Dec 19 16:01:42 PST 2022

jdoerfert added subscribers: jhuber6, tianshilei1992, josemonsalve2.
jdoerfert added a comment.

> I could not find source declarations for:
>
> __kmpc_get_hardware_num_blocks()       // __kmpc_get_hardware_num_threads_in_block looks similar and is returning uint

We somehow dropped this one and stopped using it. It was introduced in https://reviews.llvm.org/D106390, @josemonsalve2 can you take a look to re-introduce it (separate review).
>From the current functions we use, uint32 is the return type.

> __tgt_target_data_begin_mapper_issue() // looked at hide_mem_transfer_latency.ll

The impl. is under review (waiting for me) https://reviews.llvm.org/D133832. All signed as far as I can see.

> __kmpc_is_generic_main_thread_id()     // guessing it's returning int8_t, like e.g. kmpc_is_spmd_exec_mode, guessing arg is int32_t

We reordered the runtime, @jhuber6 can you check if we still need this and the folding code?

> __tgt_target_data_begin_mapper_wait()  // No narrow int param, so no worries

same as the issue method above, I think.

> __kmpc_parallel_level()                     // Called in parallel_level_fold.ll without params..?

@tianshilei1992 Can you check the uses and tests for this please.

> __tgt_target_data_begin_nowait_mapper()
> __tgt_target_data_end_nowait_mapper()
> __tgt_target_data_update_nowait_mapper()

These were introduced w/o all arguments, copy-paste error based on the non-nowait versions. We'll just need to add the corresponding arguments (@jhuber6, @tianshilei1992, @josemonsalve2).

Are there other problems with the OpenMP functions we can help with?

================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:400
+__OMP_RTL(__tgt_interop_init, false, Void, IdentPtr, Int32, VoidPtrPtr, Int32,
+          Int32, Int64, VoidPtr, Int32)
 __OMP_RTL(__tgt_interop_destroy, false, Void, IdentPtr, Int32, VoidPtrPtr,
----------------
We should ensure the enum size is 32 explicitly but this looks like a nice bugfix.

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

https://reviews.llvm.org/D133949