[Openmp-commits] [PATCH] D131309: [Libomptarget] Add utility functions for loading an ELF symbol by name

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

Implementation looks reasonable. Extra const annotations are fine. I'm not totally convinced implementation complexity has gone down - we're down 70 lines and up 210 - what're the users of the second format of hashtable, and of the linear scan? I'm guessing cuda?



================
Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:1617
+  if (!SymOrErr) {
+    REPORT("Failed ELF lookup: %s\n", toString(SymOrErr.takeError()).c_str());
     return 1;
----------------
REPORT -> DP for consistency with the rest of the plugin I think


================
Comment at: openmp/libomptarget/plugins/common/elf_common/ELFSymbols.cpp:135
+  }
+
+  return nullptr;
----------------
So one of these is presumably dead, since the only caller is amdgpu. Does cuda use the other sort of hash table? If not, suggest we delete the unused code path for now


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131309