[Openmp-commits] [PATCH] D14253: [OpenMP] Initial implementation of OpenMP offloading library - libomptarget plugins.

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added a comment.

Some minor last comments



================
Comment at: libomptarget/plugins/cuda/CMakeLists.txt:22
+    
+      if(CMAKE_BUILD_TYPE MATCHES Debug)
+          add_definitions(-DCUDA_ERROR_REPORT)
----------------
grokos wrote:
> Hahnfeld wrote:
> > `LIBOMPTARGET_CMAKE_BUILD_TYPE` to match against lowercase?
> Done.
I think this should then be lowercase `debug` to exactly match what's done in the parent directory


================
Comment at: libomptarget/plugins/cuda/src/rtl.cpp:25-31
+#ifndef TARGET_NAME
+#define TARGET_NAME Generic - 64bit
+#endif
+
+#define GETNAME2(name) #name
+#define GETNAME(name) GETNAME2(name)
+#define DP(...) DEBUGP("Target " GETNAME(TARGET_NAME) " RTL", __VA_ARGS__)
----------------
grokos wrote:
> Hahnfeld wrote:
> > Can `TARGET_NAME` be hardcoded to `CUDA`?
> TARGET_NAME is set via cmake. We could hadcode it, but the current scheme gives the flexibility to set the name at will (e.g. add version info, etc.) without tampering with the source code.
All right. But please change the fallback to say `CUDA`


================
Comment at: libomptarget/plugins/generic-elf-64bit/src/rtl.cpp:129-170
+  // Is the library version incompatible with the header file?
+  if (elf_version(EV_CURRENT) == EV_NONE) {
+    DP("Incompatible ELF library!\n");
+    return 0;
+  }
+
+  char *img_begin = (char *)image->ImageStart;
----------------
grokos wrote:
> Hahnfeld wrote:
> > This looks the same as in the CUDA plugin. Can all this be refactored into a function?
> True, it's the very same code but it just happens to be so. Another plugin for a different architecture may implement this interface function in a different way. Plugins are meant to be developed independently from each other and their code should not reply upon the existence of some common functionality / refactored code.
Maybe we can put a function like `elf_check_machine(__tgt_device_image *image, uint16_t target)` into a file `plugins/common/elf.c` and compile it into both plugins? The problem with duplicated code is that you always forget to sync them if you change something.


https://reviews.llvm.org/D14253