[Openmp-commits] [PATCH] D102692: [AMDGPU][Libomptarget] Move Kernel/Symbol info tables to RTLDeviceInfoTy

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added inline comments.
Herald added a subscriber: foad.


================
Comment at: openmp/libomptarget/plugins/amdgpu/impl/system.cpp:816
 
-static hsa_status_t get_code_object_custom_metadata(void *binary,
-                                                    size_t binSize, int gpu) {
+static hsa_status_t get_code_object_custom_metadata(
+    std::map<std::string, atl_kernel_info_t> &KernelInfoTable, void *binary,
----------------
Minor point, output parameters at the end of the list is probably more common than at the start of the list for C++.

I'd rather pass mutable things by pointer, so the call site looks like function(42, &something_mutated), but that convention is from a different codebase to this one.

Better than either would be returning a pair<status, map> instead of mutating it in place. But given this change is moving from a global variable to a parameter, getting rid of the mutation at the same time seems a step too far for one patch.


================
Comment at: openmp/libomptarget/plugins/amdgpu/impl/system.cpp:1006
                                         hsa_executable_symbol_t symbol,
                                         void *data) {
+  PopulateInfoTablesContext *Ctx =
----------------
Would lean towards dropping the void*, e.g. via D103030:
```
static hsa_status_t populate_InfoTables(hsa_executable_symbol_t symbol, 
  std::map<std::string, atl_kernel_info_t> &KernelInfoTable,
  std::map<std::string, atl_symbol_info_t> &SymbolInfoTable) {```



================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:89
 
+atmi_status_t atmi_interop_hsa_get_symbol_info(
+    std::map<std::string, atl_symbol_info_t> &SymbolInfoTable,
----------------
This seems orthogonal to moving the global.

Also, looks like we now have declarations in rtl.cpp and definitions in atmi_interop_hsa.cpp, which seems bad as we lose the compiler checking that the call site matches the definition


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102692